[PATCH] x86/Intel: don't swap operands of MONITOR and MWAIT

[PATCH] x86/Intel: don't swap operands of MONITOR and MWAIT

[Jan Beulich]

[-- Attachment #1: Type: text/plain, Size: 1122 bytes --]

Generally, the documentation doesn't allow for any explicit operands
to be specified with MONITOR/MWAIT. To permit the more legible
overriding of the address size via specifying operands, the option is
being retained even in Intel mode, but operand swapping is being
suppressed by this patch. This is both because it makes no sense here
(all of the operands are inputs) and since not doing so would require
a more intrusive fix to process_immext() (as it currently mis-numbers
the operands in the diagnostic when in Intel mode).

2012-07-24  Jan Beulich <jbeulich@suse.com>

	* config/tc-i386.c (md_assemble): Also suppress operand
	swapping for MONITOR and MWAIT.

--- a/gas/config/tc-i386.c
+++ b/gas/config/tc-i386.c
@@ -3100,6 +3100,8 @@ md_assemble (char *line)
       && i.operands > 1
       && (strcmp (mnemonic, "bound") != 0)
       && (strcmp (mnemonic, "invlpga") != 0)
+      && (strcmp (mnemonic, "monitor") != 0)
+      && (strcmp (mnemonic, "mwait") != 0)
       && !(operand_type_check (i.types[0], imm)
 	   && operand_type_check (i.types[1], imm)))
     swap_operands ();




[-- Attachment #2: binutils-mainline-x86-intel-monitor-no-swap.patch --]
[-- Type: text/plain, Size: 1116 bytes --]

Generally, the documentation doesn't allow for any explicit operands
to be specified with MONITOR/MWAIT. To permit the more legible
overriding of the address size via specifying operands, the option is
being retained even in Intel mode, but operand swapping is being
suppressed by this patch. This is both because it makes no sense here
(all of the operands are inputs) and since not doing so would require
a more intrusive fix to process_immext() (as it currently mis-numbers
the operands in the diagnostic when in Intel mode).

2012-07-24  Jan Beulich <jbeulich@suse.com>

	* config/tc-i386.c (md_assemble): Also suppress operand
	swapping for MONITOR and MWAIT.

--- a/gas/config/tc-i386.c
+++ b/gas/config/tc-i386.c
@@ -3100,6 +3100,8 @@ md_assemble (char *line)
       && i.operands > 1
       && (strcmp (mnemonic, "bound") != 0)
       && (strcmp (mnemonic, "invlpga") != 0)
+      && (strcmp (mnemonic, "monitor") != 0)
+      && (strcmp (mnemonic, "mwait") != 0)
       && !(operand_type_check (i.types[0], imm)
 	   && operand_type_check (i.types[1], imm)))
     swap_operands ();

Re: [PATCH] x86/Intel: don't swap operands of MONITOR and MWAIT

[H.J. Lu]

On Tue, Jul 24, 2012 at 7:49 AM, Jan Beulich <JBeulich@suse.com> wrote:
> Generally, the documentation doesn't allow for any explicit operands
> to be specified with MONITOR/MWAIT. To permit the more legible
> overriding of the address size via specifying operands, the option is
> being retained even in Intel mode, but operand swapping is being
> suppressed by this patch. This is both because it makes no sense here
> (all of the operands are inputs) and since not doing so would require
> a more intrusive fix to process_immext() (as it currently mis-numbers
> the operands in the diagnostic when in Intel mode).
>
> 2012-07-24  Jan Beulich <jbeulich@suse.com>
>
>         * config/tc-i386.c (md_assemble): Also suppress operand
>         swapping for MONITOR and MWAIT.
>
> --- a/gas/config/tc-i386.c
> +++ b/gas/config/tc-i386.c
> @@ -3100,6 +3100,8 @@ md_assemble (char *line)
>        && i.operands > 1
>        && (strcmp (mnemonic, "bound") != 0)
>        && (strcmp (mnemonic, "invlpga") != 0)
> +      && (strcmp (mnemonic, "monitor") != 0)
> +      && (strcmp (mnemonic, "mwait") != 0)
>        && !(operand_type_check (i.types[0], imm)
>            && operand_type_check (i.types[1], imm)))
>      swap_operands ();
>

Will it break GCC:

(define_insn "sse3_monitor"
  [(unspec_volatile [(match_operand:SI 0 "register_operand" "a")
                     (match_operand:SI 1 "register_operand" "c")
                     (match_operand:SI 2 "register_operand" "d")]
                    UNSPECV_MONITOR)]
  "TARGET_SSE3 && !TARGET_64BIT"
  "monitor\t%0, %1, %2"
  [(set_attr "length" "3")])



-- 
H.J.

Re: [PATCH] x86/Intel: don't swap operands of MONITOR and MWAIT

[Jan Beulich]

>>> On 30.07.12 at 18:20, "H.J. Lu" <hjl.tools@gmail.com> wrote:
> On Tue, Jul 24, 2012 at 7:49 AM, Jan Beulich <JBeulich@suse.com> wrote:
>> Generally, the documentation doesn't allow for any explicit operands
>> to be specified with MONITOR/MWAIT. To permit the more legible
>> overriding of the address size via specifying operands, the option is
>> being retained even in Intel mode, but operand swapping is being
>> suppressed by this patch. This is both because it makes no sense here
>> (all of the operands are inputs) and since not doing so would require
>> a more intrusive fix to process_immext() (as it currently mis-numbers
>> the operands in the diagnostic when in Intel mode).
>>
>> 2012-07-24  Jan Beulich <jbeulich@suse.com>
>>
>>         * config/tc-i386.c (md_assemble): Also suppress operand
>>         swapping for MONITOR and MWAIT.
>>
>> --- a/gas/config/tc-i386.c
>> +++ b/gas/config/tc-i386.c
>> @@ -3100,6 +3100,8 @@ md_assemble (char *line)
>>        && i.operands > 1
>>        && (strcmp (mnemonic, "bound") != 0)
>>        && (strcmp (mnemonic, "invlpga") != 0)
>> +      && (strcmp (mnemonic, "monitor") != 0)
>> +      && (strcmp (mnemonic, "mwait") != 0)
>>        && !(operand_type_check (i.types[0], imm)
>>            && operand_type_check (i.types[1], imm)))
>>      swap_operands ();
>>
> 
> Will it break GCC:
> 
> (define_insn "sse3_monitor"
>   [(unspec_volatile [(match_operand:SI 0 "register_operand" "a")
>                      (match_operand:SI 1 "register_operand" "c")
>                      (match_operand:SI 2 "register_operand" "d")]
>                     UNSPECV_MONITOR)]
>   "TARGET_SSE3 && !TARGET_64BIT"
>   "monitor\t%0, %1, %2"
>   [(set_attr "length" "3")])

Hmm, yes, quite obviously. Though imo it's a mistake for gcc to
redundantly specify the operands in the first place.

If this is indeed an issue, then I'll withdraw the patch, but would
like to see the diagnostic fixed.

Plus I'd like to add that years ago (iirc before monitor was even
introduced) I already pointed out that the operand swapping
behavior of gas is pretty inconsistent - the original AT&T syntax
was never formally specified anywhere afaik, and hence it was
never clear how _they_ intended their operands to be swapped
compared to the official Intel/AMD documentation: The
exceptions to the original swapping implied to me that the only
thing intended was to get the destination (if any) placed last,
while the order of source operands should remain the same.
This, however, got violated in a number of cases in the course
of the addition of new instructions (particularly such with more
than two operands).

Bottom line - imo the bug was introduced when monitor/mwait
got added, and gcc having got adapted to the wrong behavior
is a rather lame excuse now.

Jan

Re: [PATCH] x86/Intel: don't swap operands of MONITOR and MWAIT

[H.J. Lu]

On Mon, Jul 30, 2012 at 11:39 PM, Jan Beulich <JBeulich@suse.com> wrote:
>>>> On 30.07.12 at 18:20, "H.J. Lu" <hjl.tools@gmail.com> wrote:
>> On Tue, Jul 24, 2012 at 7:49 AM, Jan Beulich <JBeulich@suse.com> wrote:
>>> Generally, the documentation doesn't allow for any explicit operands
>>> to be specified with MONITOR/MWAIT. To permit the more legible
>>> overriding of the address size via specifying operands, the option is
>>> being retained even in Intel mode, but operand swapping is being
>>> suppressed by this patch. This is both because it makes no sense here
>>> (all of the operands are inputs) and since not doing so would require
>>> a more intrusive fix to process_immext() (as it currently mis-numbers
>>> the operands in the diagnostic when in Intel mode).
>>>
>>> 2012-07-24  Jan Beulich <jbeulich@suse.com>
>>>
>>>         * config/tc-i386.c (md_assemble): Also suppress operand
>>>         swapping for MONITOR and MWAIT.
>>>
>>> --- a/gas/config/tc-i386.c
>>> +++ b/gas/config/tc-i386.c
>>> @@ -3100,6 +3100,8 @@ md_assemble (char *line)
>>>        && i.operands > 1
>>>        && (strcmp (mnemonic, "bound") != 0)
>>>        && (strcmp (mnemonic, "invlpga") != 0)
>>> +      && (strcmp (mnemonic, "monitor") != 0)
>>> +      && (strcmp (mnemonic, "mwait") != 0)
>>>        && !(operand_type_check (i.types[0], imm)
>>>            && operand_type_check (i.types[1], imm)))
>>>      swap_operands ();
>>>
>>
>> Will it break GCC:
>>
>> (define_insn "sse3_monitor"
>>   [(unspec_volatile [(match_operand:SI 0 "register_operand" "a")
>>                      (match_operand:SI 1 "register_operand" "c")
>>                      (match_operand:SI 2 "register_operand" "d")]
>>                     UNSPECV_MONITOR)]
>>   "TARGET_SSE3 && !TARGET_64BIT"
>>   "monitor\t%0, %1, %2"
>>   [(set_attr "length" "3")])
>
> Hmm, yes, quite obviously. Though imo it's a mistake for gcc to
> redundantly specify the operands in the first place.
>
> If this is indeed an issue, then I'll withdraw the patch, but would
> like to see the diagnostic fixed.
>

Please open a bug report and we can investigate what we
can do.

Thanks.

-- 
H.J.