Re: [PATCH 3/3] x86: clean up after removal of support for gcc <= 2.8.1

["H.J. Lu" <hjl.tools@gmail.com>]

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.