[PATCH] x86: rework UWRMSR operand swapping

[PATCH] x86: rework UWRMSR operand swapping

[Jan Beulich]

As indicated during review already, doing the swapping early is overall
cheaper than doing it only after operand matching.

--- a/gas/config/tc-i386.c
+++ b/gas/config/tc-i386.c
@@ -5201,10 +5201,14 @@ md_assemble (char *line)
 	   && operand_type_check (i.types[1], imm)))
     swap_operands ();
 
-  /* The order of the immediates should be reversed
-     for 2 immediates extrq and insertq instructions */
-  if (i.imm_operands == 2
-      && (t->mnem_off == MN_extrq || t->mnem_off == MN_insertq))
+  /* The order of the immediates should be reversed for 2-immediates extrq
+     and insertq instructions.  Also UWRMSR wants its immediate to be in the
+     "canonical" place (first), despite it appearing last (in AT&T syntax, or
+     because of the swapping above) in the incoming set of operands.  */
+  if ((i.imm_operands == 2
+       && (t->mnem_off == MN_extrq || t->mnem_off == MN_insertq))
+      || (t->mnem_off == MN_uwrmsr && i.imm_operands
+	  && i.operands > i.imm_operands))
       swap_2_operands (0, 1);
 
   if (i.imm_operands)
@@ -7557,17 +7561,6 @@ match_template (char mnem_suffix)
       break;
     }
 
-  /* This pattern aims to put the unusually placed imm operand to a usual
-     place. The constraints are currently only adapted to uwrmsr, and may
-     need further tweaking when new similar instructions become available.  */
-  if (i.imm_operands && i.imm_operands < i.operands
-      && operand_type_check (operand_types[i.operands - 1], imm))
-    {
-      i.tm.operand_types[0] = operand_types[i.operands - 1];
-      i.tm.operand_types[i.operands - 1] = operand_types[0];
-      swap_2_operands(0, i.operands - 1);
-    }
-
   return t;
 }
 
--- a/opcodes/i386-opc.tbl
+++ b/opcodes/i386-opc.tbl
@@ -3354,6 +3354,8 @@ eretu, 0xf30f01ca, FRED&x64, NoSuf, {}
 urdmsr, 0xf20f38f8, USER_MSR|x64, RegMem|NoSuf|NoRex64, { Reg64, Reg64 }
 urdmsr, 0xf2f8/0, USER_MSR|x64, Modrm|Vex128|VexMap7|VexW0|NoSuf, { Imm32, Reg64 }
 uwrmsr, 0xf30f38f8, USER_MSR|x64, Modrm|NoSuf|NoRex64, { Reg64, Reg64 }
-uwrmsr, 0xf3f8/0, USER_MSR|x64, Modrm|Vex128|VexMap7|VexW0|NoSuf, { Reg64, Imm32 }
+// Immediates want to be first; md_assemble() takes care of swapping operands
+// accordingly.
+uwrmsr, 0xf3f8/0, USER_MSR|x64, Modrm|Vex128|VexMap7|VexW0|NoSuf, { Imm32, Reg64 }
 
 // USER_MSR instructions end.

RE: [PATCH] x86: rework UWRMSR operand swapping

[Hu, Lin1]

I'm OK. So, the order of operands in .tbl isn't required to be consistent with the doc.

BRs,
Lin

> -----Original Message-----
> From: Jan Beulich <jbeulich@suse.com>
> Sent: Friday, November 3, 2023 9:02 PM
> To: Binutils <binutils@sourceware.org>
> Cc: H.J. Lu <hjl.tools@gmail.com>; Hu, Lin1 <lin1.hu@intel.com>
> Subject: [PATCH] x86: rework UWRMSR operand swapping
> 
> As indicated during review already, doing the swapping early is overall cheaper
> than doing it only after operand matching.
> 
> --- a/gas/config/tc-i386.c
> +++ b/gas/config/tc-i386.c
> @@ -5201,10 +5201,14 @@ md_assemble (char *line)
>  	   && operand_type_check (i.types[1], imm)))
>      swap_operands ();
> 
> -  /* The order of the immediates should be reversed
> -     for 2 immediates extrq and insertq instructions */
> -  if (i.imm_operands == 2
> -      && (t->mnem_off == MN_extrq || t->mnem_off == MN_insertq))
> +  /* The order of the immediates should be reversed for 2-immediates extrq
> +     and insertq instructions.  Also UWRMSR wants its immediate to be in the
> +     "canonical" place (first), despite it appearing last (in AT&T syntax, or
> +     because of the swapping above) in the incoming set of operands.
> +*/
> +  if ((i.imm_operands == 2
> +       && (t->mnem_off == MN_extrq || t->mnem_off == MN_insertq))
> +      || (t->mnem_off == MN_uwrmsr && i.imm_operands
> +	  && i.operands > i.imm_operands))
>        swap_2_operands (0, 1);
> 
>    if (i.imm_operands)
> @@ -7557,17 +7561,6 @@ match_template (char mnem_suffix)
>        break;
>      }
> 
> -  /* This pattern aims to put the unusually placed imm operand to a usual
> -     place. The constraints are currently only adapted to uwrmsr, and may
> -     need further tweaking when new similar instructions become available.  */
> -  if (i.imm_operands && i.imm_operands < i.operands
> -      && operand_type_check (operand_types[i.operands - 1], imm))
> -    {
> -      i.tm.operand_types[0] = operand_types[i.operands - 1];
> -      i.tm.operand_types[i.operands - 1] = operand_types[0];
> -      swap_2_operands(0, i.operands - 1);
> -    }
> -
>    return t;
>  }
> 
> --- a/opcodes/i386-opc.tbl
> +++ b/opcodes/i386-opc.tbl
> @@ -3354,6 +3354,8 @@ eretu, 0xf30f01ca, FRED&x64, NoSuf, {}  urdmsr,
> 0xf20f38f8, USER_MSR|x64, RegMem|NoSuf|NoRex64, { Reg64, Reg64 }
> urdmsr, 0xf2f8/0, USER_MSR|x64, Modrm|Vex128|VexMap7|VexW0|NoSuf,
> { Imm32, Reg64 }  uwrmsr, 0xf30f38f8, USER_MSR|x64,
> Modrm|NoSuf|NoRex64, { Reg64, Reg64 } -uwrmsr, 0xf3f8/0, USER_MSR|x64,
> Modrm|Vex128|VexMap7|VexW0|NoSuf, { Reg64, Imm32 }
> +// Immediates want to be first; md_assemble() takes care of swapping
> +operands // accordingly.
> +uwrmsr, 0xf3f8/0, USER_MSR|x64, Modrm|Vex128|VexMap7|VexW0|NoSuf,
> {
> +Imm32, Reg64 }
> 
>  // USER_MSR instructions end.

Re: [PATCH] x86: rework UWRMSR operand swapping

[Jan Beulich]

On 06.11.2023 02:19, Hu, Lin1 wrote:
> I'm OK. So, the order of operands in .tbl isn't required to be consistent with the doc.

Well, in our code we are free to implement things how we see fit. When things
are different from the doc, of course some commenting is usually advisable.

Jan