RE: [PATCH 2/4] x86/APX: respect {vex}/{vex3}

["Cui, Lili" <lili.cui@intel.com>]

> On 18.02.2024 08:55, Cui, Lili wrote:
> >> Even when an EVEX encoding is available, use of such a prefix ought
> >> to be respected (resulting in an error) rather than ignored. As
> >> requested during review already, introduce a new encoding enumerator
> >> to record use of eGPR- s, and update state transitions accordingly.
> >>
> >
> > Yes, we have such issue for dual VEX/EVEX templates.
> >
> >> The optimize_encoding() change also addresses an internal assembler
> >> error that was previously raised when respective memory operands used
> >> eGPR-s for addressing.
> >>
> >> While this results in a change of diagnostic issued for VEX-encoded
> >> insns, the new one is at least no worse than the prior one.
> >> ---
> >> Question is whether for the state transitions we want to introduce a
> >> couple of helper functions: check_register() has duplicates each of
> >> what
> >> RC_SAE_specifier() and check_VecOperations() also do.
> >>
> >> --- a/gas/config/tc-i386.c
> >> +++ b/gas/config/tc-i386.c
> >> @@ -439,9 +439,6 @@ struct _i386_insn
> >>      /* Prefer the REX2 prefix in encoding.  */
> >>      bool rex2_encoding;
> >>
> >> -    /* Need to use an Egpr capable encoding (REX2 or EVEX).  */
> >> -    bool has_egpr;
> >> -
> >>      /* Disable instruction size optimization.  */
> >>      bool no_optimize;
> >>
> >> @@ -451,6 +448,7 @@ struct _i386_insn
> >>  	encoding_default = 0,
> >>  	encoding_vex,
> >>  	encoding_vex3,
> >> +	encoding_egpr, /* REX2 or EVEX.  */
> >
> > I find it difficult to understand putting egpr here.  Although this area can be
> further optimized, it is difficult to say that this solution is clearer than the
> current one.
> >
> > 1. We have separated vex/evex and rex/rex2, and put the state containing
> both rex2 and evex in vex/evex encoding, so that the logic becomes confusing.
> > 2. In this enumeration, each enumeration represents an encoding format,
> and only encoding_egpr describes the register of the operand.
> 
> I don't view it like this: This enumerator indicates "need an encoding which can
> represent eGPR-s, i.e. REX2 or EVEX". To me encoding_rex2_or_evex would be
> pretty clearly worse a name for it.
> 

I think the essential problem is that it's strange to put it in this enumeration.

> > 3. encoding_egpr is not the final encoding expression, but an intermediate
> state that ultimately needs to be converted into other expressions of the same
> level.
> 
> Not much different from encoding_evex512.

Yes, encoding_evex512 is also an intermediate state, at least it chooses one of the states in this enumeration. 

    /* Prefer the REX byte in encoding.  */
    bool rex_encoding;

    /* Prefer the REX2 prefix in encoding.  */
    bool rex2_encoding;

    /* Need to use an Egpr capable encoding (REX2 or EVEX).  */
    bool has_egpr;

    /* How to encode vector instructions.  */
    enum
      {
        vex_encoding_default = 0,
        vex_encoding_vex,
        vex_encoding_vex3,
        vex_encoding_evex,
        vex_encoding_evex512,
        vex_encoding_error
      } vec_encoding;


> 
> > If this patch just wants to report an error to vex prefix, maybe we could
> handle it like this. Or create a separate branch.
> >
> > --- a/gas/config/tc-i386.c
> > +++ b/gas/config/tc-i386.c
> > @@ -8322,7 +8322,8 @@ VEX_check_encoding (const insn_template *t)
> >        return 0;
> >      }
> >
> > -  if (!t->opcode_modifier.vex)
> > +  if (!t->opcode_modifier.vex
> > +      || ((i.vec_encoding == vex_encoding_vex) && i.has_egpr))
> >      {
> >        /* This instruction template doesn't have VEX prefix.  */
> >        if (i.vec_encoding != vex_encoding_default)
> 
> That's indeed a possibility, I think. Yet already when reviewing the original
> work of yours I indicated that I'd like the encoding restrictions all be
> represented by a single enum. To me it is more difficult to follow when there
> are two separate entities which need to be consulted in order to reflect all
> constraints.
> 

Egpr does conflict with the existing architecture, making implementation awkward.

> >> +.*:211: Error: no VEX/XOP encoding for `and'
> >> +.*:212: Error: no VEX/XOP encoding for `and'
> >> +.*:213: Error: .* `and'
> >> +.*:214: Error: no VEX/XOP encoding for `and'
> >> +.*:215: Error: no VEX/XOP encoding for `and'
> >> +.*:216: Error: .* `and'
> >> +.*:219: Error: .* `andn'
> 
> Please pay attention to the gap in line numbers here; that ...
> 
> >> --- a/gas/testsuite/gas/i386/x86-64-apx-egpr-inval.s
> >> +++ b/gas/testsuite/gas/i386/x86-64-apx-egpr-inval.s
> >> @@ -207,3 +207,13 @@
> >>  	vtestpd (%r27),%ymm6
> >>  	vtestps (%r27),%xmm6
> >>  	vtestps (%r27),%ymm6
> >> +# {vex}
> >> +	{vex} and %eax, %eax
> >> +	{vex} and %r8, %r8
> >> +	{vex} and %r16, %r16
> >> +	{vex} and %eax, %eax, %eax
> >> +	{vex} and %r8, %r8, %r8
> >> +	{vex} and %r16, %r16, %r16
> >> +	{vex} andn %eax, %eax, %eax
> >> +	{vex} andn %r8, %r8, %r8
> >
> > These two test cases are valid.
> 
> ... reflects exactly this fact.
> 

Normally we don't put valid test cases into invalid test case file, right?

Thanks,
Lili.