diff mbox series

x86: rework UWRMSR operand swapping

Message ID 05f45405-17c9-4e52-84e7-87c428a6ee56@suse.com
State Unresolved
Headers
Series x86: rework UWRMSR operand swapping |

Checks

Context Check Description
snail/binutils-gdb-check warning Git am fail log

Commit Message

Jan Beulich Nov. 3, 2023, 1:02 p.m. UTC 
  As indicated during review already, doing the swapping early is overall
cheaper than doing it only after operand matching.

Comments

Hu, Lin1 Nov. 6, 2023, 1:19 a.m. UTC | #1 
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.
Jan Beulich Nov. 6, 2023, 7:32 a.m. UTC | #2 
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
diff mbox series

Patch

--- 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.