[PATCH 2/2] Support APX zero-upper

[Cui, Lili]

> On 15.05.2024 08:31, Cui, Lili wrote:
> > gas/ChangeLog:
> >
> >         * config/tc-i386.c (build_apx_evex_prefix): Handle ZU.
> >         * testsuite/gas/i386/x86-64.exp: Added new tests for ZU.
> >         * testsuite/gas/i386/x86-64.exp: Added new tests for ZU.
> >         * testsuite/gas/i386/x86-64-apx-zu-intel.d: New test.
> >         * testsuite/gas/i386/x86-64-apx-zu-inval.l: Ditto.
> >         * testsuite/gas/i386/x86-64-apx-zu-inval.s: Ditto.
> >         * testsuite/gas/i386/x86-64-apx-zu.d: Ditto.
> >         * testsuite/gas/i386/x86-64-apx-zu.s: Ditto.
> >
> > opcodes/ChangeLog:
> >
> >         * i386-dis-evex-prefix.h: Handle PREFIX_EVEX_MAP4_40 ~
> >         PREFIX_EVEX_MAP4_4F.
> >         * i386-dis-evex.h: Ditto.
> >         * i386-dis.c (struct dis386): Add new micro 'ZU'.
> >         (putop): Handle %ZU.
> >         * i386-gen.c: Added ZU.
> >         * i386-opc.h: Ditto.
> >         * i386-opc.tbl: Added new templates to support ZU.
> > ---
> >  gas/config/tc-i386.c                         |   5 +-
> >  gas/testsuite/gas/i386/x86-64-apx-zu-intel.d |  78 +++++++++++++
> > gas/testsuite/gas/i386/x86-64-apx-zu-inval.l |  25 +++++
> > gas/testsuite/gas/i386/x86-64-apx-zu-inval.s |  28 +++++
> >  gas/testsuite/gas/i386/x86-64-apx-zu.d       |  78 +++++++++++++
> >  gas/testsuite/gas/i386/x86-64-apx-zu.s       |  73 ++++++++++++
> >  gas/testsuite/gas/i386/x86-64.exp            |   3 +
> >  opcodes/i386-dis-evex-prefix.h               | 112 +++++++++++++++++++
> >  opcodes/i386-dis-evex.h                      |  36 +++---
> >  opcodes/i386-dis.c                           |  35 ++++++
> >  opcodes/i386-gen.c                           |   1 +
> >  opcodes/i386-opc.h                           |   4 +
> >  opcodes/i386-opc.tbl                         |   6 +
> >  13 files changed, 464 insertions(+), 20 deletions(-)  create mode
> > 100644 gas/testsuite/gas/i386/x86-64-apx-zu-intel.d
> >  create mode 100644 gas/testsuite/gas/i386/x86-64-apx-zu-inval.l
> >  create mode 100644 gas/testsuite/gas/i386/x86-64-apx-zu-inval.s
> >  create mode 100644 gas/testsuite/gas/i386/x86-64-apx-zu.d
> >  create mode 100644 gas/testsuite/gas/i386/x86-64-apx-zu.s
> >
> 
> This is (at least) v2, isn't it? Just to repeat an earlier request: Please provide a
> brief summary of what has changed from the prior version, to aid reviewers.
> 

They are placed in patch 0/2 cover letters.

A bug was found when adding operand %ah to an invalid test case, so patch 1/2 was added to fix it. And made the following changes to the old patch.

 1. Removed two redundant judgment codes in zu.
 2. Added various types of register sizes in invalid test cases (found a bug about AH/BH/CH/DH).
 3. Removed IMUL_Fixup and added a macros 'ZU' for imul and setcc in EVEX format.
 4. Added VexWIG to EVEX format setzu/set to remove an ugly judgement.
 5. Added more test cases for imulzu and setzu.

> > @@ -10877,6 +10894,24 @@ putop (instr_info *ins, const char
> *in_template, int sizeflag)
> >  	      abort ();
> >  	    }
> >  	  break;
> > +	case 'U':
> > +	  if (l == 1 && (last[0] == 'Z'))
> > +	    {
> > +	      /* Although IMUL/SETcc does not support NDD, the EVEX.ND bit is
> > +		 used to control whether its destination register has its upper
> > +		 bits zeroed when OSIZE is 16b/8b.  */
> > +	      if (ins->vex.nd)
> > +		{
> > +		  oappend (ins, "zu");
> > +		  /* When we print zu for the EVEX instruction, we no longer
> > +		     need prefix {evex}. */
> > +		  if (evex_printed == true && startswith (ins->obufp, "{evex}"))
> > +		    ins->obufp += 6;
> 
> Is this (a) necessary and (b) correct? I ask in particular because I'd have
> expected an increment by 7 to also skip the separating blank. Things being
> okay with 6 suggests this path isn't really taken (or tested). The extra
> startswith() check also looks somewhat questionable: If evex_printed is true,
> there ought to be an {evex} prefix. It may not be at the start, but then you'd
> still want to zap it (if any zapping is needed here in the first place).
> 
> As to necessary: Isn't what you're trying to undo here the output of {evex}
> pretty close to the top of the function? That's under !ins->vex.nd, while here
> you check that ins->vex.nd is true. IOW both can't happen at the same time. It
> also can't be the one output for %NF, as that's evex-from-vex only.
> 
> Assuming it's still needed for a reason I can't spot right now: I also have to
> admit I'm slightly wary of such an increment. Elsewhere we always re-arrange
> buffer contents, I think.
> 

Yes, you are right, the 'ins->vex.nd' of imulzu and setzu are true and both are placed in MAP4, putop will not print {evex} for them, I will delete this redundant code.

I mistakenly thought putop would print {evex} for them here.

      switch (*p)
        {
        default:
          if (ins->evex_type == evex_from_legacy && !ins->vex.nd
              && !(ins->rex2 & 7) && !evex_printed)
            {
              oappend (ins, "{evex} ");
              evex_printed = true;
            }
          *ins->obufp++ = *p;
          break;

> > @@ -529,6 +533,8 @@ loopne, 0xe0, x64,
> > JumpByte|No_bSuf|No_wSuf|No_sSuf|NoRex64, { Disp8 }
> >
> >  // Set byte on flag instructions.
> >  set<cc>, 0xf9<cc:opc>/0, i386,
> Modrm|No_wSuf|No_lSuf|No_sSuf|No_qSuf,
> > { Reg8|Unspecified|BaseIndex }
> > +set<cc>, 0xf24<cc:opc>/0, APX_F,
> > +Modrm|No_wSuf|No_lSuf|No_sSuf|No_qSuf|VexWIG|EVexMap4, { Reg8 }
> > +setzu<cc>, 0xf24<cc:opc>/0, APX_F,
> > +Modrm|No_wSuf|No_lSuf|No_sSuf|No_qSuf|VexWIG|EVexMap4|ZU,
> { Reg8 }
> 
> If you really insist on not adding the extended form right away that I've been
> keeping to ask for, please say a sentence or two towards that in the (sadly so
> far again empty) description.
> 
OK.

Thanks,
Lili.