[PATCH v2] Support APX CCMP and CTEST

[Cui, Lili]

> On 03.06.2024 12:29, Cui, Lili wrote:
> > For the encoder part, add function check_Scc_OszcOperation to parse '{
> > dfv=of , sf, sf, cf}', store scc in the lower 4 bits of base_opcode,
> > and adjust base_opcode to its normal meaning before install_template.
> 
> Looks like you're now doing this in install_template(), not before?
> 

Done.

> > --- a/gas/config/tc-i386.c
> > +++ b/gas/config/tc-i386.c
> > @@ -416,6 +416,16 @@ struct _i386_insn
> >      /* Compressed disp8*N attribute.  */
> >      unsigned int memshift;
> >
> > +    /* SCC = EVEX.[SC3,SC2,SC1,SC0].  */
> > +    unsigned int scc;
> > +
> > +    /* Store 4 bits of EVEX.[OF,SF,ZF,CF].  */ #define OSZC_CF 0
> > +#define OSZC_ZF 1 #define OSZC_SF 2 #define OSZC_OF 3
> 
> I should have spotted this before: Are you actually using the bit _positions_
> anywhere? Else ...
> 
> > +    unsigned int oszc_flags;
> > +
> >      /* Prefer load or store in encoding.  */
> >      enum
> >        {
> > @@ -1929,6 +1939,98 @@ static INLINE bool need_evex_encoding (const
> > insn_template *t)  #define CPU_FLAGS_PERFECT_MATCH \
> >    (CPU_FLAGS_ARCH_MATCH | CPU_FLAGS_64BIT_MATCH)
> >
> > +static INLINE void set_oszc_flags (unsigned int oszc_shift) {
> > +  if (i.oszc_flags & (1 << oszc_shift))
> 
> ... the shift here and below can be avoided.
> 

Done.

> > +    as_bad (_("same oszc flag used twice"));
> > +  i.oszc_flags |= 1 << oszc_shift;
> > +}
> 
> I also think this diagnostic would better be limited to once per parsed insn.
> 

Do you mean to put it at the end of parse_insn ?

> > +/* Handle SCC OSZC flags.  */
> > +
> > +static int
> > +check_Scc_OszcOperations (const char *l) {
> > +  const char *suffix_string = l;
> > +  while (is_space_char (*suffix_string))
> > +    suffix_string++;
> > +
> > +  /* If {oszc flags} is absent, just return l.  */  if
> > + (*suffix_string != '{')
> > +    return 0;
> 
> Hmm, the comment says l is being returned here?
> 

Done.

> > +  /* Parse '{dfv='.  */
> > +  while (*suffix_string)
> > +    {
> > +      if (is_space_char (*suffix_string) || *suffix_string == '{')
> > +	suffix_string++;
> 
> Why would we want to accept multiple consecutive { on a single insn?
> You found the { ahead of the loop. Just increment the pointer there, look for
> only blanks here.
> 

Done.

> > +      else if (*suffix_string == '=')
> > +	{
> > +	  suffix_string++;
> > +	  break;
> > +	}
> > +      else
> > +	{
> > +	  if (startswith (suffix_string, "dfv"))
> > +	    suffix_string += 3;
> > +	  else
> > +	    {
> > +	      as_bad (_("Unrecognized oszc flags"));
> > +	      return -1;
> > +	    }
> > +	}
> 
> Please avoid unnecessarily deep indentation here. You already have one "else
> if". Just make the startswith() check another one.
> 
> As to the diagnostic: You haven't parsed "dfv=" yet, so mentioning "oszc" may
> be misleading. We simply don't know what kind of pseudo- suffix this is, so
> perhaps that's also what the diagnostic should say.
> 
> Additionally, just like for { you also want to refuse multiple "dfv" for a single
> insn.
> 

Added check for multiple 'dfv' and test case.

> > +    }
> > +
> > +  /* Parse 'of , sf, zf, cf}'.  */
> > +  while (*suffix_string)
> > +    {
> > +      if (*suffix_string == ',' || is_space_char (*suffix_string))
> > +	suffix_string++;
> > +      else if (*suffix_string == '}')
> > +	{
> > +	  suffix_string++;
> > +	  return suffix_string - l;
> > +	}
> > +      else
> > +	{
> > +	  /* For oszc flags are updated as follows:
> > +	     – OF = EVEX.OF
> > +	     – SF = EVEX.SF
> > +	     – ZF = EVEX.ZF
> > +	     – CF = EVEX.CF
> > +	     – PF = EVEX.CF
> > +	     – AF = 0.  */
> > +	  if (suffix_string[1] != 'f')
> > +	    {
> > +	      as_bad (_("Unrecognized oszc flags"));
> > +	      return -1;
> > +	    }
> > +	  switch (suffix_string[0])
> > +	    {
> > +	    case 'o':
> > +	      set_oszc_flags (OSZC_OF);
> > +	      break;
> > +	    case 's':
> > +	      set_oszc_flags (OSZC_SF);
> > +	      break;
> > +	    case 'z':
> > +	      set_oszc_flags (OSZC_ZF);
> > +	      break;
> > +	    case 'c':
> > +	      set_oszc_flags (OSZC_CF);
> > +	      break;
> > +	    default:
> > +	      as_bad (_("Unrecognized oszc flags"));
> > +	      return -1;
> > +	    }
> > +	  suffix_string += 2;
> > +	}
> > +    }
> > +
> > +  as_bad (_("Unbalanced parenthesis in suffix"));
> 
> Maybe better use `}' here. To me parenthesis means ( and ) only, but I'm aware
> that people use that, brace, and bracket in mixed associations. Hence best to
> be explicit about what is meant.
> 

Changed it to '}'.  I'm not sure if it should be `}'?

> > @@ -3793,10 +3895,19 @@ install_template (const insn_template *t)
> >  	}
> >      }
> >
> > +  /* For CCMP and CTEST the template has SCC in base_opcode. Move it out
> of
> > +     there, to then adjust base_opcode to obtain its normal meaning.
> > + */  if (i.tm.opcode_modifier.operandconstraint == SCC)
> 
> With the very use of SCC here, the comment might better disambiguate things
> and e.g. say "... has EVEX.SCC encode in base_opcode."
> 

Done.

> > +    {
> > +      /* Store scc in the lower 4 bits of base_opcode.  */
> > +      i.scc = i.tm.base_opcode & 0xf;
> > +      i.tm.base_opcode >>= 8;
> 
> If there is a comment, it wants to be accurate. The code does not do what the
> comment says.
> 
Changed it to

/* Get EVEX.SCC value from the lower 4 bits of base_opcode.  */

> > @@ -4290,6 +4401,18 @@ build_apx_evex_prefix (void)
> >        || i.tm.opcode_modifier.zu)
> >      i.vex.bytes[3] |= 0x10;
> >
> > +  /* Encode SCC and oszc flags bits.  */  if
> > + (i.tm.opcode_modifier.operandconstraint == SCC)
> > +    {
> > +      /* The default value of vvvv is 1111 and needs to be cleared.  */
> > +      i.vex.bytes[2] &= ~0x78;
> > +      i.vex.bytes[2] |= (i.oszc_flags << 3);
> > +      /* ND and aaa bits shold be 0.  */
> > +      know (!(i.vex.bytes[3] & ~0xe8));
> > +      /* The default value of V' is 1 and needs to be cleared.  */
> > +      i.vex.bytes[3] = (i.vex.bytes[3] & 0xf7) | i.scc;
> 
> Better ~8 or ~0x08 to be in line with the clearing further up.
> Whereas in the know() I'd be inclined to ask that you avoid ~ and use 0x17
> directly.
> 

Done.

> > --- /dev/null
> > +++ b/gas/testsuite/gas/i386/x86-64-apx-ccmp-ctest.s
> > @@ -0,0 +1,208 @@
> > +# Check 64bit APX_F CCMP and CTEST instructions
> > +
> > + .text
> > +_start:
> > +	ccmpbq   {dfv=cf} $0x7b,0x123(%r8,%rax,4)
> > +	ccmpb    {dfv=cf} %r15w,%ax
> > +	ccmpb    {dfv=cf} 0x123(%r8,%rax,4),%r15d
> > +	ccmpb    {dfv=of, cf} $0x7b,%r15w
> > +	ccmpbb   {dfv=of, cf} $0x7b,0x123(%r8,%rax,4)
> > +	ccmpbw   {dfv=of, sf, cf} $0x7b,0x123(%r8,%rax,4)
> > +	ccmpb    {dfv=of, sf, cf} %r15,0x123(%r8,%rax,4)
> > +	ccmpbw   {dfv=of, sf, zf, cf} $0x7b,0x123(%r8,%rax,4)
> > +	ccmpbq   {dfv=of, sf, zf, cf} $0x7b,0x123(%r8,%rax,4)
> > +	ccmpbl   {dfv=of, sf, zf} $0x7b,0x123(%r8,%rax,4)
> > +	ccmpb    {dfv=of, sf, zf} %r15,0x123(%r8,%rax,4)
> > +	ccmpbq   {dfv=of, sf} $0x7b,0x123(%r8,%rax,4)
> > +	ccmpb    {dfv=of, sf} 0x123(%r8,%rax,4),%r8b
> > +	ccmpbl   {dfv=of, zf, cf} $0x7b,0x123(%r8,%rax,4)
> > +	ccmpb    {dfv=of, zf, cf} %r8b,0x123(%r8,%rax,4)
> > +	ccmpbw   {dfv=of, zf} $0x7b,0x123(%r8,%rax,4)
> > +	ccmpb    {dfv=of, zf} %r8b,%dl
> > +	ccmpb    {dfv=of} %r15d,%edx
> > +	ccmpb    {dfv=of} 0x123(%r8,%rax,4),%r15w
> > +	ccmpb    {dfv=sf, cf} $0x7b,%r8b
> > +	ccmpb    {dfv=sf, cf} %r15d,0x123(%r8,%rax,4)
> > +	ccmpb    {dfv=sf, cf} 0x123(%r8,%rax,4),%r15
> > +	ccmpb    {dfv=sf, zf, cf} $0x7b,%r15d
> > +	ccmpb    {dfv=sf, zf, cf} 0x123(%r8,%rax,4),%r15w
> > +	ccmpb    {dfv=sf, zf} $0x7b,%r15d
> > +	ccmpb    {dfv=sf, zf} %r15d,0x123(%r8,%rax,4)
> > +	ccmpbq   {dfv=sf} $0x7b,0x123(%r8,%rax,4)
> > +	ccmpb    {dfv=sf} %r15,%r15
> > +	ccmpb    {dfv=sf} 0x123(%r8,%rax,4),%r15
> > +	ccmpb    {dfv=zf, cf} $0x7b,%r15
> > +	ccmpbl   {dfv=zf, cf} $0x7b,0x123(%r8,%rax,4)
> > +	ccmpb    {dfv=zf, cf} %r15w,0x123(%r8,%rax,4)
> > +	ccmpb    {dfv=zf} $0x7b,%r15
> > +	ccmpbw   {dfv=zf} $0x7b,0x123(%r8,%rax,4)
> > +	ccmpb    {dfv=zf} %r15w,0x123(%r8,%rax,4)
> > +	ccmpb    {dfv=} $0x7b,%r15w
> > +	ccmpbl   {dfv=} $0x7b,0x123(%r8,%rax,4)
> > +	ccmpb    {dfv=} 0x123(%r8,%rax,4),%r15d
> > +	ccmpo    {dfv=of} $0x7b,%r16
> > +	ccmpno   {dfv=of} $0x7b,%r17
> > +	ccmpb    {dfv=of} $0x7b,%r18
> > +	ccmpae   {dfv=of} $0x7b,%r19
> > +	ccmpe    {dfv=of} $0x7b,%r20
> > +	ccmpne   {dfv=of} $0x7b,%r21
> > +	ccmpbe   {dfv=of} $0x7b,%r22
> > +	ccmpa  {dfv=of} $0x7b,%r23
> 
> Is the odd padding here and ...
> 
> > +	ccmps    {dfv=of} $0x7b,%r24
> > +	ccmpns   {dfv=of} $0x7b,%r25
> > +	ccmpt    {dfv=of} $0x7b,%r26
> > +	ccmpf    {dfv=of} $0x7b,%r27
> > +	ccmpl    {dfv=of} $0x7b,%r28
> > +	ccmpge   {dfv=of} $0x7b,%r29
> > +	ccmple   {dfv=of} $0x7b,%r30
> > +	ccmpg  {dfv=of} $0x7b,%r31
> 
> ... here deliberate, to specifically test something unusual?
> 

This is a typo, probably introduced when I was performing a suffix replacement (changing scc to something like jcc ).

> > +	ctestb   {dfv=cf} $0x7b,%r15
> > +	ctestbw  {dfv=cf} $0x7b,0x123(%r8,%rax,4)
> > +	ctestbb  {dfv=of, cf} $0x7b,0x123(%r8,%rax,4)
> > +	ctestb   {dfv=of, cf} %r15,0x123(%r8,%rax,4)
> > +	ctestbq  {dfv=of, sf, cf} $0x7b,0x123(%r8,%rax,4)
> > +	ctestbl  {dfv=of, sf, zf, cf} $0x7b,0x123(%r8,%rax,4)
> > +	ctestbw  {dfv=of, sf, zf} $0x7b,0x123(%r8,%rax,4)
> > +	ctestbl  {dfv=of, sf} $0x7b,0x123(%r8,%rax,4)
> > +	ctestb   {dfv=of, sf} %r15w,0x123(%r8,%rax,4)
> > +	ctestbw  {dfv=of, zf, cf} $0x7b,0x123(%r8,%rax,4)
> > +	ctestb   {dfv=of, zf, cf} %r15w,0x123(%r8,%rax,4)
> > +	ctestbb  {dfv=of, zf} $0x7b,0x123(%r8,%rax,4)
> > +	ctestb   {dfv=of, zf} %r15,0x123(%r8,%rax,4)
> > +	ctestb   {dfv=of} %r15d,0x123(%r8,%rax,4)
> > +	ctestb   {dfv=of} %r8b,0x123(%r8,%rax,4)
> > +	ctestbl  {dfv=sf, cf} $0x7b,0x123(%r8,%rax,4)
> > +	ctestb   {dfv=sf, cf} %r15d,%edx
> > +	ctestb   {dfv=sf, zf, cf} %r15d,0x123(%r8,%rax,4)
> > +	ctestb   {dfv=sf, zf, cf} %r8b,%dl
> > +	ctestb   {dfv=sf, zf} $0x7b,%r15w
> > +	ctestbq  {dfv=sf, zf} $0x7b,0x123(%r8,%rax,4)
> > +	ctestb   {dfv=sf} $0x7b,%r15d
> > +	ctestbw  {dfv=sf} $0x7b,0x123(%r8,%rax,4)
> > +	ctestb   {dfv=zf, cf} $0x7b,%r8b
> > +	ctestbq  {dfv=zf, cf} $0x7b,0x123(%r8,%rax,4)
> > +	ctestbl  {dfv=zf} $0x7b,0x123(%r8,%rax,4)
> > +	ctestb   {dfv=zf} %r15,%r15
> > +	ctestbq  {dfv=} $0x7b,0x123(%r8,%rax,4)
> > +	ctestb   {dfv=} %r15w,%ax
> > +	ctesto   {dfv=of} $0x7b,%r16
> > +	ctestno  {dfv=of} $0x7b,%r17
> > +	ctestb   {dfv=of} $0x7b,%r18
> > +	ctestnb  {dfv=of} $0x7b,%r19
> > +	ctestz   {dfv=of} $0x7b,%r20
> > +	ctestnz  {dfv=of} $0x7b,%r21
> > +	ctestbe  {dfv=of} $0x7b,%r22
> > +	ctestnbe {dfv=of} $0x7b,%r23
> > +	ctests   {dfv=of} $0x7b,%r24
> > +	ctestns  {dfv=of} $0x7b,%r25
> > +	ctestt   {dfv=of} $0x7b,%r26
> > +	ctestf   {dfv=of} $0x7b,%r27
> > +	ctestl   {dfv=of} $0x7b,%r28
> > +	ctestnl  {dfv=of} $0x7b,%r29
> > +	ctestle  {dfv=of} $0x7b,%r30
> > +	ctestnle {dfv=of} $0x7b,%r31
> > +	{evex} cmp $0x7b,%r18
> > +	{evex} test $0x7b,%r18
> 
> Would be nice if each insn template was covered at least once.
> 

OK.

> Also, question: Wouldn't it make sense to also disassemble these as simple
> CMP/TEST (with {evex} prefix), at least by default (e.g. by slightly abusing
> suffix-always mode to also cover the case here)?
> 
> > @@ -10331,6 +10348,18 @@ static const char *const fgrps[][8] = {
> >    },
> >  };
> >
> > +static const char *const oszc_flags[16] = {
> > +  " {dfv=}", " {dfv=cf}", " {dfv=zf}", " {dfv=zf, cf}", " {dfv=sf}",
> > +  " {dfv=sf, cf}", " {dfv=sf, zf}", " {dfv=sf, zf, cf}", " {dfv=of}",
> > +  " {dfv=of, cf}", " {dfv=of, zf}", " {dfv=of, zf, cf}", " {dfv=of,
> > +sf}",
> > +  " {dfv=of, sf, cf}", " {dfv=of, sf, zf}", " {dfv=of, sf, zf, cf}"
> > +};
> 
> I find repeating " {dfv=" 16 times here quite odd, but I'm not going to insist
> that you reduce this redundancy.

I try to avoid repeating 16 times with this way, but the {} is split into two parts, which looks a bit strange. What are your thoughts？

 /* Add { dfv=of, sf, zf, cf} flags.  */
oappend (ins, "{ dfv=");
oappend (ins, oszc_flags[oszc_value]);

static const char *const oszc_flags[16] = {
  "}", "cf}", ...
};

> 
> > @@ -10505,16 +10534,38 @@ putop (instr_info *ins, const char
> *in_template, int sizeflag)
> >  	    abort ();
> >  	  break;
> >  	case 'C':
> > -	  if (ins->intel_syntax && !alt)
> > -	    break;
> > -	  if ((ins->prefixes & PREFIX_DATA) || (sizeflag & SUFFIX_ALWAYS))
> > +	  if (l == 0)
> >  	    {
> > -	      if (sizeflag & DFLAG)
> > -		*ins->obufp++ = ins->intel_syntax ? 'd' : 'l';
> > -	      else
> > -		*ins->obufp++ = ins->intel_syntax ? 'w' : 's';
> > -	      ins->used_prefixes |= (ins->prefixes & PREFIX_DATA);
> > +	      if (ins->intel_syntax && !alt)
> > +		break;
> > +	      if ((ins->prefixes & PREFIX_DATA) || (sizeflag & SUFFIX_ALWAYS))
> > +		{
> > +		  if (sizeflag & DFLAG)
> > +		    *ins->obufp++ = ins->intel_syntax ? 'd' : 'l';
> > +		  else
> > +		    *ins->obufp++ = ins->intel_syntax ? 'w' : 's';
> > +		  ins->used_prefixes |= (ins->prefixes & PREFIX_DATA);
> > +		}
> > +	    }
> > +	  else if (l == 1 && last[0] == 'S')
> > +	    {
> > +	      /* Add scc suffix.  */
> > +	      ins->obufp = stpcpy (ins->obufp, scc_suffix[ins->vex.scc]);
> 
> Why not oappend()?
> 

Added it and changed staging_area from 40 to 50 for some special cases that we used append () 3 times and exceeds the size of staging_area.

For example,

oappend (ins, scc_suffix[ins->vex.scc]);
oappend (ins, "(bad)");
oappend (ins, oszc_flags[oszc_value]);

> > +	      /* For SCC insns, the ND bit is required to be set to 0.  */
> > +	      if (ins->vex.nd)
> > +		{
> > +		  oappend (ins, "(bad)");
> > +		  break;
> > +		}
> > +
> > +	      /* These bits have been consumed and should be cleared or
> restored to default
> > +		 values.  */

Done.

Thanks,
Lili.