[PATCH 0/3] x86: FPU insn handling simplifications

public inbox for binutils@sourceware.org
 help / color / mirror / Atom feed

* [PATCH 0/3] x86: FPU insn handling simplifications
@ 2022-11-24  8:56 Jan Beulich
  2022-11-24  8:57 ` [PATCH 1/3] x86: extend FPU test coverage for AT&T / Intel mnemonic differences Jan Beulich
                   ` (2 more replies)
  0 siblings, 3 replies; 9+ messages in thread
From: Jan Beulich @ 2022-11-24  8:56 UTC (permalink / raw)
  To: Binutils; +Cc: H.J. Lu

Recognizing FloatR as a largely redundant attribute we can reduce
the number of (again largely redundant) FPU insn templates some.

1: extend FPU test coverage for AT&T / Intel mnemonic differences
2: drop FloatR
3: clean up after removal of support for gcc <= 2.8.1

Jan

^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH 1/3] x86: extend FPU test coverage for AT&T / Intel mnemonic differences
  2022-11-24  8:56 [PATCH 0/3] x86: FPU insn handling simplifications Jan Beulich
@ 2022-11-24  8:57 ` Jan Beulich
  2022-11-29 23:59   ` H.J. Lu
  2022-11-24  8:57 ` [PATCH 2/3] x86: drop FloatR Jan Beulich
  2022-11-24  8:58 ` [PATCH 3/3] x86: clean up after removal of support for gcc <= 2.8.1 Jan Beulich
  2 siblings, 1 reply; 9+ messages in thread
From: Jan Beulich @ 2022-11-24  8:57 UTC (permalink / raw)
  To: Binutils; +Cc: H.J. Lu

Before touching the templates, let's ensure we actually cover things:
For one FSUB{,R} and FDIV{,R} would better be tested with operands in
both possible orders. And then -mmnemonic=intel wasn't tested at all.

--- a/gas/testsuite/gas/i386/compat.d
+++ b/gas/testsuite/gas/i386/compat.d
@@ -8,18 +8,22 @@ Disassembly of section .text:
 
 0+ <.text>:
 [ 	]*[a-f0-9]+:	dc e3                	fsub   %st,%st\(3\)
+[ 	]*[a-f0-9]+:	d8 e3                	fsub   %st\(3\),%st
 [ 	]*[a-f0-9]+:	de e1                	fsubp  %st,%st\(1\)
 [ 	]*[a-f0-9]+:	de e3                	fsubp  %st,%st\(3\)
 [ 	]*[a-f0-9]+:	de e3                	fsubp  %st,%st\(3\)
 [ 	]*[a-f0-9]+:	dc eb                	fsubr  %st,%st\(3\)
+[ 	]*[a-f0-9]+:	d8 eb                	fsubr  %st\(3\),%st
 [ 	]*[a-f0-9]+:	de e9                	fsubrp %st,%st\(1\)
 [ 	]*[a-f0-9]+:	de eb                	fsubrp %st,%st\(3\)
 [ 	]*[a-f0-9]+:	de eb                	fsubrp %st,%st\(3\)
 [ 	]*[a-f0-9]+:	dc f3                	fdiv   %st,%st\(3\)
+[ 	]*[a-f0-9]+:	d8 f3                	fdiv   %st\(3\),%st
 [ 	]*[a-f0-9]+:	de f1                	fdivp  %st,%st\(1\)
 [ 	]*[a-f0-9]+:	de f3                	fdivp  %st,%st\(3\)
 [ 	]*[a-f0-9]+:	de f3                	fdivp  %st,%st\(3\)
 [ 	]*[a-f0-9]+:	dc fb                	fdivr  %st,%st\(3\)
+[ 	]*[a-f0-9]+:	d8 fb                	fdivr  %st\(3\),%st
 [ 	]*[a-f0-9]+:	de f9                	fdivrp %st,%st\(1\)
 [ 	]*[a-f0-9]+:	de fb                	fdivrp %st,%st\(3\)
 [ 	]*[a-f0-9]+:	de fb                	fdivrp %st,%st\(3\)
--- a/gas/testsuite/gas/i386/compat.s
+++ b/gas/testsuite/gas/i386/compat.s
@@ -1,18 +1,22 @@
 # Check SYSV mnemonic instructions.
 	.text 
 	fsub	%st,%st(3)
+	fsub	%st(3),%st
 	fsubp
 	fsubp	%st(3)
 	fsubp	%st,%st(3)
 	fsubr	%st,%st(3)
+	fsubr	%st(3),%st
 	fsubrp
 	fsubrp	%st(3)
 	fsubrp	%st,%st(3)
 	fdiv	%st,%st(3)
+	fdiv	%st(3),%st
 	fdivp
 	fdivp	%st(3)
 	fdivp	%st,%st(3)
 	fdivr	%st,%st(3)
+	fdivr	%st(3),%st
 	fdivrp
 	fdivrp	%st(3)
 	fdivrp	%st,%st(3)
--- a/gas/testsuite/gas/i386/compat-intel.d
+++ b/gas/testsuite/gas/i386/compat-intel.d
@@ -9,18 +9,22 @@ Disassembly of section .text:
 
 0+ <.text>:
 [ 	]*[a-f0-9]+:	dc e3                	fsubr  st\(3\),st
+[ 	]*[a-f0-9]+:	d8 e3                	fsub   st,st\(3\)
 [ 	]*[a-f0-9]+:	de e1                	fsubrp st\(1\),st
 [ 	]*[a-f0-9]+:	de e3                	fsubrp st\(3\),st
 [ 	]*[a-f0-9]+:	de e3                	fsubrp st\(3\),st
 [ 	]*[a-f0-9]+:	dc eb                	fsub   st\(3\),st
+[ 	]*[a-f0-9]+:	d8 eb                	fsubr  st,st\(3\)
 [ 	]*[a-f0-9]+:	de e9                	fsubp  st\(1\),st
 [ 	]*[a-f0-9]+:	de eb                	fsubp  st\(3\),st
 [ 	]*[a-f0-9]+:	de eb                	fsubp  st\(3\),st
 [ 	]*[a-f0-9]+:	dc f3                	fdivr  st\(3\),st
+[ 	]*[a-f0-9]+:	d8 f3                	fdiv   st,st\(3\)
 [ 	]*[a-f0-9]+:	de f1                	fdivrp st\(1\),st
 [ 	]*[a-f0-9]+:	de f3                	fdivrp st\(3\),st
 [ 	]*[a-f0-9]+:	de f3                	fdivrp st\(3\),st
 [ 	]*[a-f0-9]+:	dc fb                	fdiv   st\(3\),st
+[ 	]*[a-f0-9]+:	d8 fb                	fdivr  st,st\(3\)
 [ 	]*[a-f0-9]+:	de f9                	fdivp  st\(1\),st
 [ 	]*[a-f0-9]+:	de fb                	fdivp  st\(3\),st
 [ 	]*[a-f0-9]+:	de fb                	fdivp  st\(3\),st
--- /dev/null
+++ b/gas/testsuite/gas/i386/compat-intel2.d
@@ -0,0 +1,31 @@
+#as: -mmnemonic=intel
+#objdump: -d -Mintel-mnemonic
+#name: i386 float Intel mnemonic (2)
+#source: compat.s
+
+.*: +file format .*
+
+Disassembly of section .text:
+
+0+ <.text>:
+[ 	]*[a-f0-9]+:	dc eb                	fsub   st\(3\),st
+[ 	]*[a-f0-9]+:	d8 e3                	fsub   st,st\(3\)
+[ 	]*[a-f0-9]+:	de e9                	fsubp  st\(1\),st
+[ 	]*[a-f0-9]+:	de eb                	fsubp  st\(3\),st
+[ 	]*[a-f0-9]+:	de eb                	fsubp  st\(3\),st
+[ 	]*[a-f0-9]+:	dc e3                	fsubr  st\(3\),st
+[ 	]*[a-f0-9]+:	d8 eb                	fsubr  st,st\(3\)
+[ 	]*[a-f0-9]+:	de e1                	fsubrp st\(1\),st
+[ 	]*[a-f0-9]+:	de e3                	fsubrp st\(3\),st
+[ 	]*[a-f0-9]+:	de e3                	fsubrp st\(3\),st
+[ 	]*[a-f0-9]+:	dc fb                	fdiv   st\(3\),st
+[ 	]*[a-f0-9]+:	d8 f3                	fdiv   st,st\(3\)
+[ 	]*[a-f0-9]+:	de f9                	fdivp  st\(1\),st
+[ 	]*[a-f0-9]+:	de fb                	fdivp  st\(3\),st
+[ 	]*[a-f0-9]+:	de fb                	fdivp  st\(3\),st
+[ 	]*[a-f0-9]+:	dc f3                	fdivr  st\(3\),st
+[ 	]*[a-f0-9]+:	d8 fb                	fdivr  st,st\(3\)
+[ 	]*[a-f0-9]+:	de f1                	fdivrp st\(1\),st
+[ 	]*[a-f0-9]+:	de f3                	fdivrp st\(3\),st
+[ 	]*[a-f0-9]+:	de f3                	fdivrp st\(3\),st
+#pass
--- a/gas/testsuite/gas/i386/i386.exp
+++ b/gas/testsuite/gas/i386/i386.exp
@@ -178,6 +178,7 @@ if [gas_32_check] then {
     run_dump_test "i386-intel"
     run_dump_test "compat"
     run_dump_test "compat-intel"
+    run_dump_test "compat-intel2"
     run_dump_test "arch-1"
     run_dump_test "arch-2"
     run_dump_test "arch-3"


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 1/3] x86: extend FPU test coverage for AT&T / Intel mnemonic differences
  2022-11-24  8:57 ` [PATCH 1/3] x86: extend FPU test coverage for AT&T / Intel mnemonic differences Jan Beulich
@ 2022-11-29 23:59   ` H.J. Lu
  0 siblings, 0 replies; 9+ messages in thread
From: H.J. Lu @ 2022-11-29 23:59 UTC (permalink / raw)
  To: Jan Beulich; +Cc: Binutils

On Thu, Nov 24, 2022 at 12:57 AM Jan Beulich <jbeulich@suse.com> wrote:
>
> Before touching the templates, let's ensure we actually cover things:
> For one FSUB{,R} and FDIV{,R} would better be tested with operands in
> both possible orders. And then -mmnemonic=intel wasn't tested at all.
>
> --- a/gas/testsuite/gas/i386/compat.d
> +++ b/gas/testsuite/gas/i386/compat.d
> @@ -8,18 +8,22 @@ Disassembly of section .text:
>
>  0+ <.text>:
>  [      ]*[a-f0-9]+:    dc e3                   fsub   %st,%st\(3\)
> +[      ]*[a-f0-9]+:    d8 e3                   fsub   %st\(3\),%st
>  [      ]*[a-f0-9]+:    de e1                   fsubp  %st,%st\(1\)
>  [      ]*[a-f0-9]+:    de e3                   fsubp  %st,%st\(3\)
>  [      ]*[a-f0-9]+:    de e3                   fsubp  %st,%st\(3\)
>  [      ]*[a-f0-9]+:    dc eb                   fsubr  %st,%st\(3\)
> +[      ]*[a-f0-9]+:    d8 eb                   fsubr  %st\(3\),%st
>  [      ]*[a-f0-9]+:    de e9                   fsubrp %st,%st\(1\)
>  [      ]*[a-f0-9]+:    de eb                   fsubrp %st,%st\(3\)
>  [      ]*[a-f0-9]+:    de eb                   fsubrp %st,%st\(3\)
>  [      ]*[a-f0-9]+:    dc f3                   fdiv   %st,%st\(3\)
> +[      ]*[a-f0-9]+:    d8 f3                   fdiv   %st\(3\),%st
>  [      ]*[a-f0-9]+:    de f1                   fdivp  %st,%st\(1\)
>  [      ]*[a-f0-9]+:    de f3                   fdivp  %st,%st\(3\)
>  [      ]*[a-f0-9]+:    de f3                   fdivp  %st,%st\(3\)
>  [      ]*[a-f0-9]+:    dc fb                   fdivr  %st,%st\(3\)
> +[      ]*[a-f0-9]+:    d8 fb                   fdivr  %st\(3\),%st
>  [      ]*[a-f0-9]+:    de f9                   fdivrp %st,%st\(1\)
>  [      ]*[a-f0-9]+:    de fb                   fdivrp %st,%st\(3\)
>  [      ]*[a-f0-9]+:    de fb                   fdivrp %st,%st\(3\)
> --- a/gas/testsuite/gas/i386/compat.s
> +++ b/gas/testsuite/gas/i386/compat.s
> @@ -1,18 +1,22 @@
>  # Check SYSV mnemonic instructions.
>         .text
>         fsub    %st,%st(3)
> +       fsub    %st(3),%st
>         fsubp
>         fsubp   %st(3)
>         fsubp   %st,%st(3)
>         fsubr   %st,%st(3)
> +       fsubr   %st(3),%st
>         fsubrp
>         fsubrp  %st(3)
>         fsubrp  %st,%st(3)
>         fdiv    %st,%st(3)
> +       fdiv    %st(3),%st
>         fdivp
>         fdivp   %st(3)
>         fdivp   %st,%st(3)
>         fdivr   %st,%st(3)
> +       fdivr   %st(3),%st
>         fdivrp
>         fdivrp  %st(3)
>         fdivrp  %st,%st(3)
> --- a/gas/testsuite/gas/i386/compat-intel.d
> +++ b/gas/testsuite/gas/i386/compat-intel.d
> @@ -9,18 +9,22 @@ Disassembly of section .text:
>
>  0+ <.text>:
>  [      ]*[a-f0-9]+:    dc e3                   fsubr  st\(3\),st
> +[      ]*[a-f0-9]+:    d8 e3                   fsub   st,st\(3\)
>  [      ]*[a-f0-9]+:    de e1                   fsubrp st\(1\),st
>  [      ]*[a-f0-9]+:    de e3                   fsubrp st\(3\),st
>  [      ]*[a-f0-9]+:    de e3                   fsubrp st\(3\),st
>  [      ]*[a-f0-9]+:    dc eb                   fsub   st\(3\),st
> +[      ]*[a-f0-9]+:    d8 eb                   fsubr  st,st\(3\)
>  [      ]*[a-f0-9]+:    de e9                   fsubp  st\(1\),st
>  [      ]*[a-f0-9]+:    de eb                   fsubp  st\(3\),st
>  [      ]*[a-f0-9]+:    de eb                   fsubp  st\(3\),st
>  [      ]*[a-f0-9]+:    dc f3                   fdivr  st\(3\),st
> +[      ]*[a-f0-9]+:    d8 f3                   fdiv   st,st\(3\)
>  [      ]*[a-f0-9]+:    de f1                   fdivrp st\(1\),st
>  [      ]*[a-f0-9]+:    de f3                   fdivrp st\(3\),st
>  [      ]*[a-f0-9]+:    de f3                   fdivrp st\(3\),st
>  [      ]*[a-f0-9]+:    dc fb                   fdiv   st\(3\),st
> +[      ]*[a-f0-9]+:    d8 fb                   fdivr  st,st\(3\)
>  [      ]*[a-f0-9]+:    de f9                   fdivp  st\(1\),st
>  [      ]*[a-f0-9]+:    de fb                   fdivp  st\(3\),st
>  [      ]*[a-f0-9]+:    de fb                   fdivp  st\(3\),st
> --- /dev/null
> +++ b/gas/testsuite/gas/i386/compat-intel2.d
> @@ -0,0 +1,31 @@
> +#as: -mmnemonic=intel
> +#objdump: -d -Mintel-mnemonic
> +#name: i386 float Intel mnemonic (2)
> +#source: compat.s
> +
> +.*: +file format .*
> +
> +Disassembly of section .text:
> +
> +0+ <.text>:
> +[      ]*[a-f0-9]+:    dc eb                   fsub   st\(3\),st
> +[      ]*[a-f0-9]+:    d8 e3                   fsub   st,st\(3\)
> +[      ]*[a-f0-9]+:    de e9                   fsubp  st\(1\),st
> +[      ]*[a-f0-9]+:    de eb                   fsubp  st\(3\),st
> +[      ]*[a-f0-9]+:    de eb                   fsubp  st\(3\),st
> +[      ]*[a-f0-9]+:    dc e3                   fsubr  st\(3\),st
> +[      ]*[a-f0-9]+:    d8 eb                   fsubr  st,st\(3\)
> +[      ]*[a-f0-9]+:    de e1                   fsubrp st\(1\),st
> +[      ]*[a-f0-9]+:    de e3                   fsubrp st\(3\),st
> +[      ]*[a-f0-9]+:    de e3                   fsubrp st\(3\),st
> +[      ]*[a-f0-9]+:    dc fb                   fdiv   st\(3\),st
> +[      ]*[a-f0-9]+:    d8 f3                   fdiv   st,st\(3\)
> +[      ]*[a-f0-9]+:    de f9                   fdivp  st\(1\),st
> +[      ]*[a-f0-9]+:    de fb                   fdivp  st\(3\),st
> +[      ]*[a-f0-9]+:    de fb                   fdivp  st\(3\),st
> +[      ]*[a-f0-9]+:    dc f3                   fdivr  st\(3\),st
> +[      ]*[a-f0-9]+:    d8 fb                   fdivr  st,st\(3\)
> +[      ]*[a-f0-9]+:    de f1                   fdivrp st\(1\),st
> +[      ]*[a-f0-9]+:    de f3                   fdivrp st\(3\),st
> +[      ]*[a-f0-9]+:    de f3                   fdivrp st\(3\),st
> +#pass
> --- a/gas/testsuite/gas/i386/i386.exp
> +++ b/gas/testsuite/gas/i386/i386.exp
> @@ -178,6 +178,7 @@ if [gas_32_check] then {
>      run_dump_test "i386-intel"
>      run_dump_test "compat"
>      run_dump_test "compat-intel"
> +    run_dump_test "compat-intel2"
>      run_dump_test "arch-1"
>      run_dump_test "arch-2"
>      run_dump_test "arch-3"
>

OK

Thanks.

-- 
H.J.

^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH 2/3] x86: drop FloatR
  2022-11-24  8:56 [PATCH 0/3] x86: FPU insn handling simplifications Jan Beulich
  2022-11-24  8:57 ` [PATCH 1/3] x86: extend FPU test coverage for AT&T / Intel mnemonic differences Jan Beulich
@ 2022-11-24  8:57 ` Jan Beulich
  2022-11-30  0:00   ` H.J. Lu
  2022-11-24  8:58 ` [PATCH 3/3] x86: clean up after removal of support for gcc <= 2.8.1 Jan Beulich
  2 siblings, 1 reply; 9+ messages in thread
From: Jan Beulich @ 2022-11-24  8:57 UTC (permalink / raw)
  To: Binutils; +Cc: H.J. Lu

There are just 4 templates using it, which can be easily identified by
other means, as D is set only on a very limited number of FPU templates.
Also move the respective conditional out of the code path taken by all
"reverse match" insns (it probably should have been this way already
before, to avoid the one conditional in the common case).

With this the templates which had FloatR dropped no longer differ from
their AT&T syntax + mnemonic counterparts - the only difference is now
which of the two would be recognized. For this, however, we don't need
two templates - we can simply arrange the condition for setting
Opcode_FloatR accordingly.

--- a/gas/config/tc-i386.c
+++ b/gas/config/tc-i386.c
@@ -6800,12 +6800,18 @@ match_template (char mnem_suffix)
 		  specific_error = progress (i.error);
 		  continue;
 		}
-	      /* found_reverse_match holds which of D or FloatR
+	      /* found_reverse_match holds which variant of D
 		 we've found.  */
 	      if (!t->opcode_modifier.d)
 		found_reverse_match = 0;
 	      else if (operand_types[0].bitfield.tbyte)
-		found_reverse_match = Opcode_FloatD;
+		{
+		  found_reverse_match = Opcode_FloatD;
+		  /* FSUB{,R} and FDIV{,R} may need a 2nd bit flipped.  */
+		  if ((t->base_opcode & 0x20)
+		      && (intel_syntax || intel_mnemonic))
+		    found_reverse_match |= Opcode_FloatR;
+		}
 	      else if (t->opcode_modifier.vexsources)
 		{
 		  found_reverse_match = Opcode_VexW;
@@ -6820,8 +6826,6 @@ match_template (char mnem_suffix)
 				      ? Opcode_ExtD : Opcode_SIMD_IntD;
 	      else
 		found_reverse_match = Opcode_D;
-	      if (t->opcode_modifier.floatr)
-		found_reverse_match |= Opcode_FloatR;
 	    }
 	  else
 	    {
--- a/opcodes/i386-gen.c
+++ b/opcodes/i386-gen.c
@@ -731,7 +731,6 @@ static bitfield opcode_modifiers[] =
   BITFIELD (Modrm),
   BITFIELD (Jump),
   BITFIELD (FloatMF),
-  BITFIELD (FloatR),
   BITFIELD (Size),
   BITFIELD (CheckRegSize),
   BITFIELD (OperandConstraint),
--- a/opcodes/i386-opc.h
+++ b/opcodes/i386-opc.h
@@ -487,8 +487,6 @@ enum
   Jump,
   /* FP insn memory format bit, sized by 0x4 */
   FloatMF,
-  /* src/dest swap for floats. */
-  FloatR,
   /* needs size prefix if in 32-bit mode */
 #define SIZE16 1
   /* needs size prefix if in 16-bit mode */
@@ -743,7 +741,6 @@ typedef struct i386_opcode_modifier
   unsigned int modrm:1;
   unsigned int jump:3;
   unsigned int floatmf:1;
-  unsigned int floatr:1;
   unsigned int size:2;
   unsigned int checkregsize:1;
   unsigned int operandconstraint:4;
--- a/opcodes/i386-opc.tbl
+++ b/opcodes/i386-opc.tbl
@@ -694,11 +694,10 @@ faddp, 0xdec0, None, CpuFP, NoSuf|Ugh, {
 
 // subtract
 fsub, 0xd8e0, None, CpuFP, NoSuf, { FloatReg }
-fsub, 0xd8e0, None, CpuFP, D|NoSuf|ATTMnemonic|ATTSyntax, { FloatReg, FloatAcc }
+fsub, 0xd8e0, None, CpuFP, D|NoSuf, { FloatReg, FloatAcc }
 // alias for fsubp
 fsub, 0xdee1, None, CpuFP, NoSuf|Ugh|ATTMnemonic|ATTSyntax, {}
 fsub, 0xdee9, None, CpuFP, NoSuf|Ugh|ATTMnemonic, {}
-fsub, 0xd8e0, None, CpuFP, NoSuf|D|FloatR, { FloatReg, FloatAcc }
 fsub, 0xd8, 4, CpuFP, Modrm|FloatMF|No_bSuf|No_wSuf|No_qSuf|No_ldSuf, { Dword|Qword|Unspecified|BaseIndex }
 fisub, 0xde, 4, CpuFP, Modrm|FloatMF|No_bSuf|No_wSuf|No_qSuf|No_ldSuf, { Word|Dword|Unspecified|BaseIndex }
 
@@ -711,11 +710,10 @@ fsubp, 0xdee9, None, CpuFP, NoSuf, {}
 
 // subtract reverse
 fsubr, 0xd8e8, None, CpuFP, NoSuf, { FloatReg }
-fsubr, 0xd8e8, None, CpuFP, D|NoSuf|ATTMnemonic|ATTSyntax, { FloatReg, FloatAcc }
+fsubr, 0xd8e8, None, CpuFP, D|NoSuf, { FloatReg, FloatAcc }
 // alias for fsubrp
 fsubr, 0xdee9, None, CpuFP, NoSuf|Ugh|ATTMnemonic|ATTSyntax, {}
 fsubr, 0xdee1, None, CpuFP, NoSuf|Ugh|ATTMnemonic, {}
-fsubr, 0xd8e8, None, CpuFP, NoSuf|D|FloatR, { FloatReg, FloatAcc }
 fsubr, 0xd8, 5, CpuFP, Modrm|FloatMF|No_bSuf|No_wSuf|No_qSuf|No_ldSuf, { Dword|Qword|Unspecified|BaseIndex }
 fisubr, 0xde, 5, CpuFP, Modrm|FloatMF|No_bSuf|No_wSuf|No_qSuf|No_ldSuf, { Word|Dword|Unspecified|BaseIndex }
 
@@ -741,11 +739,10 @@ fmulp, 0xdec8, None, CpuFP, NoSuf|Ugh, {
 
 // divide
 fdiv, 0xd8f0, None, CpuFP, NoSuf, { FloatReg }
-fdiv, 0xd8f0, None, CpuFP, D|NoSuf|ATTMnemonic|ATTSyntax, { FloatReg, FloatAcc }
+fdiv, 0xd8f0, None, CpuFP, D|NoSuf, { FloatReg, FloatAcc }
 // alias for fdivp
 fdiv, 0xdef1, None, CpuFP, NoSuf|Ugh|ATTMnemonic|ATTSyntax, {}
 fdiv, 0xdef9, None, CpuFP, NoSuf|Ugh|ATTMnemonic, {}
-fdiv, 0xd8f0, None, CpuFP, NoSuf|D|FloatR, { FloatReg, FloatAcc }
 fdiv, 0xd8, 6, CpuFP, Modrm|FloatMF|No_bSuf|No_wSuf|No_qSuf|No_ldSuf, { Dword|Qword|Unspecified|BaseIndex }
 fidiv, 0xde, 6, CpuFP, Modrm|FloatMF|No_bSuf|No_wSuf|No_qSuf|No_ldSuf, { Word|Dword|Unspecified|BaseIndex }
 
@@ -758,11 +755,10 @@ fdivp, 0xdef9, None, CpuFP, NoSuf, {}
 
 // divide reverse
 fdivr, 0xd8f8, None, CpuFP, NoSuf, { FloatReg }
-fdivr, 0xd8f8, None, CpuFP, D|NoSuf|ATTMnemonic|ATTSyntax, { FloatReg, FloatAcc }
+fdivr, 0xd8f8, None, CpuFP, D|NoSuf, { FloatReg, FloatAcc }
 // alias for fdivrp
 fdivr, 0xdef9, None, CpuFP, NoSuf|Ugh|ATTMnemonic|ATTSyntax, {}
 fdivr, 0xdef1, None, CpuFP, NoSuf|Ugh|ATTMnemonic, {}
-fdivr, 0xd8f8, None, CpuFP, NoSuf|D|FloatR, { FloatReg, FloatAcc }
 fdivr, 0xd8, 7, CpuFP, Modrm|FloatMF|No_bSuf|No_wSuf|No_qSuf|No_ldSuf, { Dword|Qword|Unspecified|BaseIndex }
 fidivr, 0xde, 7, CpuFP, Modrm|FloatMF|No_bSuf|No_wSuf|No_qSuf|No_ldSuf, { Word|Dword|Unspecified|BaseIndex }
 


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 2/3] x86: drop FloatR
  2022-11-24  8:57 ` [PATCH 2/3] x86: drop FloatR Jan Beulich
@ 2022-11-30  0:00   ` H.J. Lu
  0 siblings, 0 replies; 9+ messages in thread
From: H.J. Lu @ 2022-11-30  0:00 UTC (permalink / raw)
  To: Jan Beulich; +Cc: Binutils

On Thu, Nov 24, 2022 at 12:57 AM Jan Beulich <jbeulich@suse.com> wrote:
>
> There are just 4 templates using it, which can be easily identified by
> other means, as D is set only on a very limited number of FPU templates.
> Also move the respective conditional out of the code path taken by all
> "reverse match" insns (it probably should have been this way already
> before, to avoid the one conditional in the common case).
>
> With this the templates which had FloatR dropped no longer differ from
> their AT&T syntax + mnemonic counterparts - the only difference is now
> which of the two would be recognized. For this, however, we don't need
> two templates - we can simply arrange the condition for setting
> Opcode_FloatR accordingly.
>
> --- a/gas/config/tc-i386.c
> +++ b/gas/config/tc-i386.c
> @@ -6800,12 +6800,18 @@ match_template (char mnem_suffix)
>                   specific_error = progress (i.error);
>                   continue;
>                 }
> -             /* found_reverse_match holds which of D or FloatR
> +             /* found_reverse_match holds which variant of D
>                  we've found.  */
>               if (!t->opcode_modifier.d)
>                 found_reverse_match = 0;
>               else if (operand_types[0].bitfield.tbyte)
> -               found_reverse_match = Opcode_FloatD;
> +               {
> +                 found_reverse_match = Opcode_FloatD;
> +                 /* FSUB{,R} and FDIV{,R} may need a 2nd bit flipped.  */
> +                 if ((t->base_opcode & 0x20)
> +                     && (intel_syntax || intel_mnemonic))
> +                   found_reverse_match |= Opcode_FloatR;
> +               }
>               else if (t->opcode_modifier.vexsources)
>                 {
>                   found_reverse_match = Opcode_VexW;
> @@ -6820,8 +6826,6 @@ match_template (char mnem_suffix)
>                                       ? Opcode_ExtD : Opcode_SIMD_IntD;
>               else
>                 found_reverse_match = Opcode_D;
> -             if (t->opcode_modifier.floatr)
> -               found_reverse_match |= Opcode_FloatR;
>             }
>           else
>             {
> --- a/opcodes/i386-gen.c
> +++ b/opcodes/i386-gen.c
> @@ -731,7 +731,6 @@ static bitfield opcode_modifiers[] =
>    BITFIELD (Modrm),
>    BITFIELD (Jump),
>    BITFIELD (FloatMF),
> -  BITFIELD (FloatR),
>    BITFIELD (Size),
>    BITFIELD (CheckRegSize),
>    BITFIELD (OperandConstraint),
> --- a/opcodes/i386-opc.h
> +++ b/opcodes/i386-opc.h
> @@ -487,8 +487,6 @@ enum
>    Jump,
>    /* FP insn memory format bit, sized by 0x4 */
>    FloatMF,
> -  /* src/dest swap for floats. */
> -  FloatR,
>    /* needs size prefix if in 32-bit mode */
>  #define SIZE16 1
>    /* needs size prefix if in 16-bit mode */
> @@ -743,7 +741,6 @@ typedef struct i386_opcode_modifier
>    unsigned int modrm:1;
>    unsigned int jump:3;
>    unsigned int floatmf:1;
> -  unsigned int floatr:1;
>    unsigned int size:2;
>    unsigned int checkregsize:1;
>    unsigned int operandconstraint:4;
> --- a/opcodes/i386-opc.tbl
> +++ b/opcodes/i386-opc.tbl
> @@ -694,11 +694,10 @@ faddp, 0xdec0, None, CpuFP, NoSuf|Ugh, {
>
>  // subtract
>  fsub, 0xd8e0, None, CpuFP, NoSuf, { FloatReg }
> -fsub, 0xd8e0, None, CpuFP, D|NoSuf|ATTMnemonic|ATTSyntax, { FloatReg, FloatAcc }
> +fsub, 0xd8e0, None, CpuFP, D|NoSuf, { FloatReg, FloatAcc }
>  // alias for fsubp
>  fsub, 0xdee1, None, CpuFP, NoSuf|Ugh|ATTMnemonic|ATTSyntax, {}
>  fsub, 0xdee9, None, CpuFP, NoSuf|Ugh|ATTMnemonic, {}
> -fsub, 0xd8e0, None, CpuFP, NoSuf|D|FloatR, { FloatReg, FloatAcc }
>  fsub, 0xd8, 4, CpuFP, Modrm|FloatMF|No_bSuf|No_wSuf|No_qSuf|No_ldSuf, { Dword|Qword|Unspecified|BaseIndex }
>  fisub, 0xde, 4, CpuFP, Modrm|FloatMF|No_bSuf|No_wSuf|No_qSuf|No_ldSuf, { Word|Dword|Unspecified|BaseIndex }
>
> @@ -711,11 +710,10 @@ fsubp, 0xdee9, None, CpuFP, NoSuf, {}
>
>  // subtract reverse
>  fsubr, 0xd8e8, None, CpuFP, NoSuf, { FloatReg }
> -fsubr, 0xd8e8, None, CpuFP, D|NoSuf|ATTMnemonic|ATTSyntax, { FloatReg, FloatAcc }
> +fsubr, 0xd8e8, None, CpuFP, D|NoSuf, { FloatReg, FloatAcc }
>  // alias for fsubrp
>  fsubr, 0xdee9, None, CpuFP, NoSuf|Ugh|ATTMnemonic|ATTSyntax, {}
>  fsubr, 0xdee1, None, CpuFP, NoSuf|Ugh|ATTMnemonic, {}
> -fsubr, 0xd8e8, None, CpuFP, NoSuf|D|FloatR, { FloatReg, FloatAcc }
>  fsubr, 0xd8, 5, CpuFP, Modrm|FloatMF|No_bSuf|No_wSuf|No_qSuf|No_ldSuf, { Dword|Qword|Unspecified|BaseIndex }
>  fisubr, 0xde, 5, CpuFP, Modrm|FloatMF|No_bSuf|No_wSuf|No_qSuf|No_ldSuf, { Word|Dword|Unspecified|BaseIndex }
>
> @@ -741,11 +739,10 @@ fmulp, 0xdec8, None, CpuFP, NoSuf|Ugh, {
>
>  // divide
>  fdiv, 0xd8f0, None, CpuFP, NoSuf, { FloatReg }
> -fdiv, 0xd8f0, None, CpuFP, D|NoSuf|ATTMnemonic|ATTSyntax, { FloatReg, FloatAcc }
> +fdiv, 0xd8f0, None, CpuFP, D|NoSuf, { FloatReg, FloatAcc }
>  // alias for fdivp
>  fdiv, 0xdef1, None, CpuFP, NoSuf|Ugh|ATTMnemonic|ATTSyntax, {}
>  fdiv, 0xdef9, None, CpuFP, NoSuf|Ugh|ATTMnemonic, {}
> -fdiv, 0xd8f0, None, CpuFP, NoSuf|D|FloatR, { FloatReg, FloatAcc }
>  fdiv, 0xd8, 6, CpuFP, Modrm|FloatMF|No_bSuf|No_wSuf|No_qSuf|No_ldSuf, { Dword|Qword|Unspecified|BaseIndex }
>  fidiv, 0xde, 6, CpuFP, Modrm|FloatMF|No_bSuf|No_wSuf|No_qSuf|No_ldSuf, { Word|Dword|Unspecified|BaseIndex }
>
> @@ -758,11 +755,10 @@ fdivp, 0xdef9, None, CpuFP, NoSuf, {}
>
>  // divide reverse
>  fdivr, 0xd8f8, None, CpuFP, NoSuf, { FloatReg }
> -fdivr, 0xd8f8, None, CpuFP, D|NoSuf|ATTMnemonic|ATTSyntax, { FloatReg, FloatAcc }
> +fdivr, 0xd8f8, None, CpuFP, D|NoSuf, { FloatReg, FloatAcc }
>  // alias for fdivrp
>  fdivr, 0xdef9, None, CpuFP, NoSuf|Ugh|ATTMnemonic|ATTSyntax, {}
>  fdivr, 0xdef1, None, CpuFP, NoSuf|Ugh|ATTMnemonic, {}
> -fdivr, 0xd8f8, None, CpuFP, NoSuf|D|FloatR, { FloatReg, FloatAcc }
>  fdivr, 0xd8, 7, CpuFP, Modrm|FloatMF|No_bSuf|No_wSuf|No_qSuf|No_ldSuf, { Dword|Qword|Unspecified|BaseIndex }
>  fidivr, 0xde, 7, CpuFP, Modrm|FloatMF|No_bSuf|No_wSuf|No_qSuf|No_ldSuf, { Word|Dword|Unspecified|BaseIndex }
>
>

OK.

Thanks.

-- 
H.J.

^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH 3/3] x86: clean up after removal of support for gcc <= 2.8.1
  2022-11-24  8:56 [PATCH 0/3] x86: FPU insn handling simplifications Jan Beulich
  2022-11-24  8:57 ` [PATCH 1/3] x86: extend FPU test coverage for AT&T / Intel mnemonic differences Jan Beulich
  2022-11-24  8:57 ` [PATCH 2/3] x86: drop FloatR Jan Beulich
@ 2022-11-24  8:58 ` Jan Beulich
  2022-11-28 23:21   ` H.J. Lu
  2022-11-30  0:00   ` H.J. Lu
  2 siblings, 2 replies; 9+ messages in thread
From: Jan Beulich @ 2022-11-24  8:58 UTC (permalink / raw)
  To: Binutils; +Cc: H.J. Lu

At the very least a comment in process_operands() is stale. Beyond that
there are effectively two options:
1) It is possible that FADDP and FMULP were mistakenly not marked as
   being in need of dealing with the compiler anomaly, and hence the
   respective templates weren't removed at the time when they should
   have been.
2) It is also possible that there are indeed uses known beyond compiler
   generated output for these two commutative opcodes, and hence the
   templates need to stay.
To be on the safe side assume 2: Update the comment and fold the
templates into their "normal" ones (utilizing D), adjusting consuming
code accordingly.

For FMULP also add a comment paralleling a similar one FADDP has.

--- a/gas/config/tc-i386.c
+++ b/gas/config/tc-i386.c
@@ -6806,7 +6806,8 @@ match_template (char mnem_suffix)
 		found_reverse_match = 0;
 	      else if (operand_types[0].bitfield.tbyte)
 		{
-		  found_reverse_match = Opcode_FloatD;
+		  if (t->opcode_modifier.operandconstraint != UGH)
+		    found_reverse_match = Opcode_FloatD;
 		  /* FSUB{,R} and FDIV{,R} may need a 2nd bit flipped.  */
 		  if ((t->base_opcode & 0x20)
 		      && (intel_syntax || intel_mnemonic))
@@ -7997,29 +7998,31 @@ process_operands (void)
     {
       /* The register or float register operand is in operand
 	 0 or 1.  */
-      unsigned int op = i.tm.operand_types[0].bitfield.class != Reg;
+      const reg_entry *r = i.op[0].regs;
 
+      if (i.imm_operands
+	  || (r->reg_type.bitfield.instance == Accum && i.op[1].regs))
+	r = i.op[1].regs;
       /* Register goes in low 3 bits of opcode.  */
-      i.tm.base_opcode |= i.op[op].regs->reg_num;
-      if ((i.op[op].regs->reg_flags & RegRex) != 0)
+      i.tm.base_opcode |= r->reg_num;
+      if ((r->reg_flags & RegRex) != 0)
 	i.rex |= REX_B;
       if (!quiet_warnings && i.tm.opcode_modifier.operandconstraint == UGH)
 	{
-	  /* Warn about some common errors, but press on regardless.
-	     The first case can be generated by gcc (<= 2.8.1).  */
-	  if (i.operands == 2)
-	    {
-	      /* Reversed arguments on faddp, fsubp, etc.  */
-	      as_warn (_("translating to `%s %s%s,%s%s'"), i.tm.name,
-		       register_prefix, i.op[!intel_syntax].regs->reg_name,
-		       register_prefix, i.op[intel_syntax].regs->reg_name);
-	    }
-	  else
+	  /* Warn about some common errors, but press on regardless.  */
+	  if (i.operands != 2)
 	    {
 	      /* Extraneous `l' suffix on fp insn.  */
 	      as_warn (_("translating to `%s %s%s'"), i.tm.name,
 		       register_prefix, i.op[0].regs->reg_name);
 	    }
+	  else if (i.op[0].regs->reg_type.bitfield.instance != Accum)
+	    {
+	      /* Reversed arguments on faddp or fmulp.  */
+	      as_warn (_("translating to `%s %s%s,%s%s'"), i.tm.name,
+		       register_prefix, i.op[!intel_syntax].regs->reg_name,
+		       register_prefix, i.op[intel_syntax].regs->reg_name);
+	    }
 	}
     }
 
--- a/opcodes/i386-opc.tbl
+++ b/opcodes/i386-opc.tbl
@@ -686,11 +686,10 @@ fadd, 0xdec1, None, CpuFP, NoSuf|Ugh|ATT
 fadd, 0xd8, 0, CpuFP, Modrm|FloatMF|No_bSuf|No_wSuf|No_qSuf|No_ldSuf, { Dword|Qword|Unspecified|BaseIndex }
 fiadd, 0xde, 0, CpuFP, Modrm|FloatMF|No_bSuf|No_wSuf|No_qSuf|No_ldSuf, { Word|Dword|Unspecified|BaseIndex }
 
-faddp, 0xdec0, None, CpuFP, NoSuf, { FloatAcc, FloatReg }
+faddp, 0xdec0, None, CpuFP, D|NoSuf|Ugh, { FloatAcc, FloatReg }
 faddp, 0xdec0, None, CpuFP, NoSuf, { FloatReg }
 // alias for faddp %st, %st(1)
 faddp, 0xdec1, None, CpuFP, NoSuf, {}
-faddp, 0xdec0, None, CpuFP, NoSuf|Ugh, { FloatReg, FloatAcc }
 
 // subtract
 fsub, 0xd8e0, None, CpuFP, NoSuf, { FloatReg }
@@ -732,10 +731,10 @@ fmul, 0xdec9, None, CpuFP, NoSuf|Ugh|ATT
 fmul, 0xd8, 1, CpuFP, Modrm|FloatMF|No_bSuf|No_wSuf|No_qSuf|No_ldSuf, { Dword|Qword|Unspecified|BaseIndex }
 fimul, 0xde, 1, CpuFP, Modrm|FloatMF|No_bSuf|No_wSuf|No_qSuf|No_ldSuf, { Word|Dword|Unspecified|BaseIndex }
 
-fmulp, 0xdec8, None, CpuFP, NoSuf, { FloatAcc, FloatReg }
+fmulp, 0xdec8, None, CpuFP, D|NoSuf|Ugh, { FloatAcc, FloatReg }
 fmulp, 0xdec8, None, CpuFP, NoSuf, { FloatReg }
+// alias for fmulp %st, %st(1)
 fmulp, 0xdec9, None, CpuFP, NoSuf, {}
-fmulp, 0xdec8, None, CpuFP, NoSuf|Ugh, { FloatReg, FloatAcc }
 
 // divide
 fdiv, 0xd8f0, None, CpuFP, NoSuf, { FloatReg }


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 3/3] x86: clean up after removal of support for gcc <= 2.8.1
  2022-11-24  8:58 ` [PATCH 3/3] x86: clean up after removal of support for gcc <= 2.8.1 Jan Beulich
@ 2022-11-28 23:21   ` H.J. Lu
  2022-11-29  9:02     ` Jan Beulich
  2022-11-30  0:00   ` H.J. Lu
  1 sibling, 1 reply; 9+ messages in thread
From: H.J. Lu @ 2022-11-28 23:21 UTC (permalink / raw)
  To: Jan Beulich; +Cc: Binutils

On Thu, Nov 24, 2022 at 12:58 AM Jan Beulich <jbeulich@suse.com> wrote:
>
> At the very least a comment in process_operands() is stale. Beyond that
> there are effectively two options:
> 1) It is possible that FADDP and FMULP were mistakenly not marked as
>    being in need of dealing with the compiler anomaly, and hence the
>    respective templates weren't removed at the time when they should
>    have been.
> 2) It is also possible that there are indeed uses known beyond compiler
>    generated output for these two commutative opcodes, and hence the
>    templates need to stay.
> To be on the safe side assume 2: Update the comment and fold the
> templates into their "normal" ones (utilizing D), adjusting consuming
> code accordingly.
>
> For FMULP also add a comment paralleling a similar one FADDP has.

Please mention dropping GCC 2.8.1 support in gas/NEWS.

> --- a/gas/config/tc-i386.c
> +++ b/gas/config/tc-i386.c
> @@ -6806,7 +6806,8 @@ match_template (char mnem_suffix)
>                 found_reverse_match = 0;
>               else if (operand_types[0].bitfield.tbyte)
>                 {
> -                 found_reverse_match = Opcode_FloatD;
> +                 if (t->opcode_modifier.operandconstraint != UGH)
> +                   found_reverse_match = Opcode_FloatD;
>                   /* FSUB{,R} and FDIV{,R} may need a 2nd bit flipped.  */
>                   if ((t->base_opcode & 0x20)
>                       && (intel_syntax || intel_mnemonic))
> @@ -7997,29 +7998,31 @@ process_operands (void)
>      {
>        /* The register or float register operand is in operand
>          0 or 1.  */
> -      unsigned int op = i.tm.operand_types[0].bitfield.class != Reg;
> +      const reg_entry *r = i.op[0].regs;
>
> +      if (i.imm_operands
> +         || (r->reg_type.bitfield.instance == Accum && i.op[1].regs))
> +       r = i.op[1].regs;
>        /* Register goes in low 3 bits of opcode.  */
> -      i.tm.base_opcode |= i.op[op].regs->reg_num;
> -      if ((i.op[op].regs->reg_flags & RegRex) != 0)
> +      i.tm.base_opcode |= r->reg_num;
> +      if ((r->reg_flags & RegRex) != 0)
>         i.rex |= REX_B;
>        if (!quiet_warnings && i.tm.opcode_modifier.operandconstraint == UGH)
>         {
> -         /* Warn about some common errors, but press on regardless.
> -            The first case can be generated by gcc (<= 2.8.1).  */
> -         if (i.operands == 2)
> -           {
> -             /* Reversed arguments on faddp, fsubp, etc.  */
> -             as_warn (_("translating to `%s %s%s,%s%s'"), i.tm.name,
> -                      register_prefix, i.op[!intel_syntax].regs->reg_name,
> -                      register_prefix, i.op[intel_syntax].regs->reg_name);
> -           }
> -         else
> +         /* Warn about some common errors, but press on regardless.  */
> +         if (i.operands != 2)
>             {
>               /* Extraneous `l' suffix on fp insn.  */
>               as_warn (_("translating to `%s %s%s'"), i.tm.name,
>                        register_prefix, i.op[0].regs->reg_name);
>             }
> +         else if (i.op[0].regs->reg_type.bitfield.instance != Accum)
> +           {
> +             /* Reversed arguments on faddp or fmulp.  */
> +             as_warn (_("translating to `%s %s%s,%s%s'"), i.tm.name,
> +                      register_prefix, i.op[!intel_syntax].regs->reg_name,
> +                      register_prefix, i.op[intel_syntax].regs->reg_name);
> +           }
>         }
>      }
>
> --- a/opcodes/i386-opc.tbl
> +++ b/opcodes/i386-opc.tbl
> @@ -686,11 +686,10 @@ fadd, 0xdec1, None, CpuFP, NoSuf|Ugh|ATT
>  fadd, 0xd8, 0, CpuFP, Modrm|FloatMF|No_bSuf|No_wSuf|No_qSuf|No_ldSuf, { Dword|Qword|Unspecified|BaseIndex }
>  fiadd, 0xde, 0, CpuFP, Modrm|FloatMF|No_bSuf|No_wSuf|No_qSuf|No_ldSuf, { Word|Dword|Unspecified|BaseIndex }
>
> -faddp, 0xdec0, None, CpuFP, NoSuf, { FloatAcc, FloatReg }
> +faddp, 0xdec0, None, CpuFP, D|NoSuf|Ugh, { FloatAcc, FloatReg }
>  faddp, 0xdec0, None, CpuFP, NoSuf, { FloatReg }
>  // alias for faddp %st, %st(1)
>  faddp, 0xdec1, None, CpuFP, NoSuf, {}
> -faddp, 0xdec0, None, CpuFP, NoSuf|Ugh, { FloatReg, FloatAcc }
>
>  // subtract
>  fsub, 0xd8e0, None, CpuFP, NoSuf, { FloatReg }
> @@ -732,10 +731,10 @@ fmul, 0xdec9, None, CpuFP, NoSuf|Ugh|ATT
>  fmul, 0xd8, 1, CpuFP, Modrm|FloatMF|No_bSuf|No_wSuf|No_qSuf|No_ldSuf, { Dword|Qword|Unspecified|BaseIndex }
>  fimul, 0xde, 1, CpuFP, Modrm|FloatMF|No_bSuf|No_wSuf|No_qSuf|No_ldSuf, { Word|Dword|Unspecified|BaseIndex }
>
> -fmulp, 0xdec8, None, CpuFP, NoSuf, { FloatAcc, FloatReg }
> +fmulp, 0xdec8, None, CpuFP, D|NoSuf|Ugh, { FloatAcc, FloatReg }
>  fmulp, 0xdec8, None, CpuFP, NoSuf, { FloatReg }
> +// alias for fmulp %st, %st(1)
>  fmulp, 0xdec9, None, CpuFP, NoSuf, {}
> -fmulp, 0xdec8, None, CpuFP, NoSuf|Ugh, { FloatReg, FloatAcc }
>
>  // divide
>  fdiv, 0xd8f0, None, CpuFP, NoSuf, { FloatReg }
>


-- 
H.J.

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 3/3] x86: clean up after removal of support for gcc <= 2.8.1
  2022-11-28 23:21   ` H.J. Lu
@ 2022-11-29  9:02     ` Jan Beulich
  0 siblings, 0 replies; 9+ messages in thread
From: Jan Beulich @ 2022-11-29  9:02 UTC (permalink / raw)
  To: H.J. Lu; +Cc: Binutils

On 29.11.2022 00:21, H.J. Lu wrote:
> On Thu, Nov 24, 2022 at 12:58 AM Jan Beulich <jbeulich@suse.com> wrote:
>>
>> At the very least a comment in process_operands() is stale. Beyond that
>> there are effectively two options:
>> 1) It is possible that FADDP and FMULP were mistakenly not marked as
>>    being in need of dealing with the compiler anomaly, and hence the
>>    respective templates weren't removed at the time when they should
>>    have been.
>> 2) It is also possible that there are indeed uses known beyond compiler
>>    generated output for these two commutative opcodes, and hence the
>>    templates need to stay.
>> To be on the safe side assume 2: Update the comment and fold the
>> templates into their "normal" ones (utilizing D), adjusting consuming
>> code accordingly.
>>
>> For FMULP also add a comment paralleling a similar one FADDP has.
> 
> Please mention dropping GCC 2.8.1 support in gas/NEWS.

For the old release, many, many years ago, when that support was dropped?
I'm not dropping any support here, I'm merely cleaning up after that sort
of incomplete dropping. And the title also clearly says exactly that.

Jan

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 3/3] x86: clean up after removal of support for gcc <= 2.8.1
  2022-11-24  8:58 ` [PATCH 3/3] x86: clean up after removal of support for gcc <= 2.8.1 Jan Beulich
  2022-11-28 23:21   ` H.J. Lu
@ 2022-11-30  0:00   ` H.J. Lu
  1 sibling, 0 replies; 9+ messages in thread
From: H.J. Lu @ 2022-11-30  0:00 UTC (permalink / raw)
  To: Jan Beulich; +Cc: Binutils

On Thu, Nov 24, 2022 at 12:58 AM Jan Beulich <jbeulich@suse.com> wrote:
>
> At the very least a comment in process_operands() is stale. Beyond that
> there are effectively two options:
> 1) It is possible that FADDP and FMULP were mistakenly not marked as
>    being in need of dealing with the compiler anomaly, and hence the
>    respective templates weren't removed at the time when they should
>    have been.
> 2) It is also possible that there are indeed uses known beyond compiler
>    generated output for these two commutative opcodes, and hence the
>    templates need to stay.
> To be on the safe side assume 2: Update the comment and fold the
> templates into their "normal" ones (utilizing D), adjusting consuming
> code accordingly.
>
> For FMULP also add a comment paralleling a similar one FADDP has.
>
> --- a/gas/config/tc-i386.c
> +++ b/gas/config/tc-i386.c
> @@ -6806,7 +6806,8 @@ match_template (char mnem_suffix)
>                 found_reverse_match = 0;
>               else if (operand_types[0].bitfield.tbyte)
>                 {
> -                 found_reverse_match = Opcode_FloatD;
> +                 if (t->opcode_modifier.operandconstraint != UGH)
> +                   found_reverse_match = Opcode_FloatD;
>                   /* FSUB{,R} and FDIV{,R} may need a 2nd bit flipped.  */
>                   if ((t->base_opcode & 0x20)
>                       && (intel_syntax || intel_mnemonic))
> @@ -7997,29 +7998,31 @@ process_operands (void)
>      {
>        /* The register or float register operand is in operand
>          0 or 1.  */
> -      unsigned int op = i.tm.operand_types[0].bitfield.class != Reg;
> +      const reg_entry *r = i.op[0].regs;
>
> +      if (i.imm_operands
> +         || (r->reg_type.bitfield.instance == Accum && i.op[1].regs))
> +       r = i.op[1].regs;
>        /* Register goes in low 3 bits of opcode.  */
> -      i.tm.base_opcode |= i.op[op].regs->reg_num;
> -      if ((i.op[op].regs->reg_flags & RegRex) != 0)
> +      i.tm.base_opcode |= r->reg_num;
> +      if ((r->reg_flags & RegRex) != 0)
>         i.rex |= REX_B;
>        if (!quiet_warnings && i.tm.opcode_modifier.operandconstraint == UGH)
>         {
> -         /* Warn about some common errors, but press on regardless.
> -            The first case can be generated by gcc (<= 2.8.1).  */
> -         if (i.operands == 2)
> -           {
> -             /* Reversed arguments on faddp, fsubp, etc.  */
> -             as_warn (_("translating to `%s %s%s,%s%s'"), i.tm.name,
> -                      register_prefix, i.op[!intel_syntax].regs->reg_name,
> -                      register_prefix, i.op[intel_syntax].regs->reg_name);
> -           }
> -         else
> +         /* Warn about some common errors, but press on regardless.  */
> +         if (i.operands != 2)
>             {
>               /* Extraneous `l' suffix on fp insn.  */
>               as_warn (_("translating to `%s %s%s'"), i.tm.name,
>                        register_prefix, i.op[0].regs->reg_name);
>             }
> +         else if (i.op[0].regs->reg_type.bitfield.instance != Accum)
> +           {
> +             /* Reversed arguments on faddp or fmulp.  */
> +             as_warn (_("translating to `%s %s%s,%s%s'"), i.tm.name,
> +                      register_prefix, i.op[!intel_syntax].regs->reg_name,
> +                      register_prefix, i.op[intel_syntax].regs->reg_name);
> +           }
>         }
>      }
>
> --- a/opcodes/i386-opc.tbl
> +++ b/opcodes/i386-opc.tbl
> @@ -686,11 +686,10 @@ fadd, 0xdec1, None, CpuFP, NoSuf|Ugh|ATT
>  fadd, 0xd8, 0, CpuFP, Modrm|FloatMF|No_bSuf|No_wSuf|No_qSuf|No_ldSuf, { Dword|Qword|Unspecified|BaseIndex }
>  fiadd, 0xde, 0, CpuFP, Modrm|FloatMF|No_bSuf|No_wSuf|No_qSuf|No_ldSuf, { Word|Dword|Unspecified|BaseIndex }
>
> -faddp, 0xdec0, None, CpuFP, NoSuf, { FloatAcc, FloatReg }
> +faddp, 0xdec0, None, CpuFP, D|NoSuf|Ugh, { FloatAcc, FloatReg }
>  faddp, 0xdec0, None, CpuFP, NoSuf, { FloatReg }
>  // alias for faddp %st, %st(1)
>  faddp, 0xdec1, None, CpuFP, NoSuf, {}
> -faddp, 0xdec0, None, CpuFP, NoSuf|Ugh, { FloatReg, FloatAcc }
>
>  // subtract
>  fsub, 0xd8e0, None, CpuFP, NoSuf, { FloatReg }
> @@ -732,10 +731,10 @@ fmul, 0xdec9, None, CpuFP, NoSuf|Ugh|ATT
>  fmul, 0xd8, 1, CpuFP, Modrm|FloatMF|No_bSuf|No_wSuf|No_qSuf|No_ldSuf, { Dword|Qword|Unspecified|BaseIndex }
>  fimul, 0xde, 1, CpuFP, Modrm|FloatMF|No_bSuf|No_wSuf|No_qSuf|No_ldSuf, { Word|Dword|Unspecified|BaseIndex }
>
> -fmulp, 0xdec8, None, CpuFP, NoSuf, { FloatAcc, FloatReg }
> +fmulp, 0xdec8, None, CpuFP, D|NoSuf|Ugh, { FloatAcc, FloatReg }
>  fmulp, 0xdec8, None, CpuFP, NoSuf, { FloatReg }
> +// alias for fmulp %st, %st(1)
>  fmulp, 0xdec9, None, CpuFP, NoSuf, {}
> -fmulp, 0xdec8, None, CpuFP, NoSuf|Ugh, { FloatReg, FloatAcc }
>
>  // divide
>  fdiv, 0xd8f0, None, CpuFP, NoSuf, { FloatReg }
>

OK.

Thanks.

-- 
H.J.

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2022-11-30  0:01 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-11-24  8:56 [PATCH 0/3] x86: FPU insn handling simplifications Jan Beulich
2022-11-24  8:57 ` [PATCH 1/3] x86: extend FPU test coverage for AT&T / Intel mnemonic differences Jan Beulich
2022-11-29 23:59   ` H.J. Lu
2022-11-24  8:57 ` [PATCH 2/3] x86: drop FloatR Jan Beulich
2022-11-30  0:00   ` H.J. Lu
2022-11-24  8:58 ` [PATCH 3/3] x86: clean up after removal of support for gcc <= 2.8.1 Jan Beulich
2022-11-28 23:21   ` H.J. Lu
2022-11-29  9:02     ` Jan Beulich
2022-11-30  0:00   ` H.J. Lu

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).