[PATCH] Support APX CCMP and CTEST

[Cui, Lili]

> On 23.05.2024 08:12, Cui, Lili wrote:
> > With the new macro %NP (no prefix) introduced in this patch, we can
> remove %ME of movbe.
> 
> What are you describing here? I can't spot a respective change in the patch. If
> this is meant to say "we then could ...", please put such remarks outside of the
> commit message area.
> 
It's not part of the commit message, I will submit a separate patch for movbe and remove this message in the next version of the CCMP and CTEST patch.

> > CCMP and CTEST are two new sets of instructions for conditional CMP
> > and TEST, SCC and OSZC flags are given as suffixes of CCMP or CTEST in
> > the instruction mnemonic, e.g.:
> >
> > ccmp<cc> { dfv=sf , cf , of }
> >
> > 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
> > (like the legacy instructions), and restore it to i.tm.base_opcode in
> > install_template.
> 
> What do you mean by "restore it to i.tm.base_opcode"? I don't think this
> comes anywhere near what you're actually doing.
> 
> Just for the record: I also consider this syntax pretty odd, but I understand
> that's what "Assembly Syntax Recommendations" suggests. It would imo be
> quite a bit less convoluted if it simply was a separate pseudo-immediate
> operand (much like e.g. {sae} is in AT&T syntax).
> 
> > --- a/gas/config/tc-i386-intel.c
> > +++ b/gas/config/tc-i386-intel.c
> > @@ -619,6 +619,14 @@ i386_intel_operand (char *operand_string, int
> got_a_float)
> >    bool rc_sae_modifier = i.rounding.type != rc_none && i.rounding.modifier;
> >    int rc
> >
> > +  /* Handle SCC OSZC flgs.  */
> > +  if (current_templates.start->opcode_modifier.scc)
> > +    {
> > +      operand_string = check_Scc_OszcOperations (operand_string);
> > +      if (operand_string == NULL)
> > +	return 0;
> > +    }
> 
> This doesn't look right to me: We're parsing an operand here (at any position),
> yet the OSZC specifier is effectively a mnemonic pseudo- suffix (or else there
> would need to be a separating comma between it and the other operand). This
> is also shown in the doc, where this always follows the mnemonic, irrespective
> of AT&T vs Intel operand order.
> 

I previously thought that it is similar in function to SAE so I put it here, but if we emphasize that one is an operand and the other is a suffix, it is indeed more appropriate to put the latter at the end of parse_insn.

> > --- 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;
> > +
> > +    #define CF 0
> > +    #define ZF 1
> > +    #define SF 2
> > +    #define OF 3
> 
> These names are misleading - they suggest they somehow enumerate
> EFLAGS.CF etc. Perhaps OSZC_CF etc?
> 

They belong to oszc_flags, just after the definitions, I will move the comments before these definitions to avoid misunderstandings.

> Also #define-s starting in the first column, please (blanks between # and define
> are okay-ish, if so desired).
> 
Done.

> > +    /* Store 4 bits of EVEX.[OF,SF,ZF,CF].  */
> > +    unsigned int oszc_flags;
> > +
> >      /* Prefer load or store in encoding.  */
> >      enum
> >        {
> > @@ -3793,10 +3803,19 @@ install_template (const insn_template *t)
> >  	}
> >      }
> >
> > +  /* For the ccmp and ctest instructions we use base_opcode like the legacy
> > +     instructions and restore it in i.tm.base_opcode.  */  if
> > + (i.tm.opcode_modifier.scc)
> > +    {
> > +      /* Store scc in the lower 4 bits of base_opcode.  */
> > +      i.scc = i.tm.base_opcode & 0xf;
> > +      i.tm.base_opcode >>= 8;
> > +    }
> 
> As noted on the description already: You don't mean "restore" here, as there's
> no "save" counterpart anywhere. "like the legacy insns" isn't quite right either.
> Maybe
> 
>   /* 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.  */
> 
> ?

It is better, thanks.

> 
> > @@ -4290,6 +4309,16 @@ 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.scc)
> > +    {
> > +      i.vex.bytes[2] &= ~0x78;
> 
> Is there any way these 4 bits may be set before coming here? I hope there
> isn't, in which case an assertion (or know()) would seem more appropriate.
> 

They are the vvvv bits and the default value should be 1111. We need to clear them to 0000.

> > +      i.vex.bytes[2] |= (i.oszc_flags << 3);
> > +      i.vex.bytes[3] = (i.vex.bytes[3] & 0xf0) | i.scc;
> 
> Same question for these 4 bits, and ...
> 

They are V'aaa, V' defaults to 1 and needs to be cleared to 0, but we can add a check for aaa.

> > +      /* The ND bit is required to be set to 0.  */
> > +      i.vex.bytes[3] &= 0xef;
> 
> ... for ND.
> 
Together with aaa.

> > @@ -7539,13 +7568,14 @@ parse_operands (char *l, const char
> *mnemonic)
> >  		      i.operands + 1);
> >  	      return NULL;
> >  	    }
> > -	  if (!intel_syntax && !in_quotes)
> > +	  if (!in_quotes)
> >  	    {
> > -	      if (*l == '(')
> > +	      if (*l == '(' || *l == '{')
> >  		++paren_not_balanced;
> > -	      if (*l == ')')
> > +	      if (*l == ')' || *l == '}')
> >  		--paren_not_balanced;
> >  	    }
> 
> This isn't right either (closing } shouldn't match an opening parenthesis), but
> will go away anyway as you move the parsing from operand handling to
> mnemonic handling.
> 

Yes.

> > @@ -13578,6 +13608,93 @@ RC_SAE_specifier (const char *pstr)
> >    return NULL;
> >  }
> >
> > +/* Handle SCC OSZC flags.  */
> > +
> > +static char *
> > +check_Scc_OszcOperations (char *op_string) {
> > +  /* If {oszc flags} is absent, just return op_string.  */
> > +  if (*op_string != '{')
> > +    return op_string;
> > +  else
> 
> Please help limiting indentation by omitting such an "else".
> 

Done.

> > +    {
> > +      bool has_equal;
> > +      /* Parse '{dfv='.  */
> > +      while (*op_string)
> > +	{
> > +	  if (*op_string != '=')
> > +	    op_string++;
> > +	  else
> > +	    {
> > +	      has_equal = true;
> > +	      op_string++;
> > +	      break;
> > +	    }
> > +	}
> > +
> > +      if (!has_equal)
> > +	{
> > +	  /* If there is no '=', report bad.  */
> > +	  as_bad (_("Unrecognized oszc flags"));
> > +	  ignore_rest_of_line ();
> > +	}
> > +    }
> 
> Where does "dfv" actually get checked? You can't just look for '=' and hope the
> rest is fine.
> 
Changed it to check "dfv=".

> ignore_rest_of_line() also looks wrong to use here, as you're not consuming
> from input_line_pointer. Instead I think you want to return NULL there instead.
> 

Removed them.

> > +  /* Parse 'of , sf, sf, cf}'.  */
> 
> "sf" twice?
> 

Done.

> > +  while (*op_string)
> > +    {
> > +      if (*op_string == ',' || is_space_char (*op_string))
> > +	op_string++;
> > +      else if (*op_string == '}')
> > +	{
> > +	  op_string++;
> > +	  while (is_space_char (*op_string))
> > +	    op_string++;
> > +	  return op_string;
> > +	}
> > +      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 (op_string[1] != 'f')
> > +	    {
> > +	      as_bad (_("Unrecognized oszc flags"));
> > +	      ignore_rest_of_line ();
> > +	      return NULL;
> > +	    }
> > +	  switch (op_string[0])
> > +	    {
> > +	    case 'o':
> > +	      i.oszc_flags |= (1 << OF);
> > +	      break;
> > +	    case 's':
> > +	      i.oszc_flags |= (1 << SF);
> > +	      break;
> > +	    case 'z':
> > +	      i.oszc_flags |= (1 << ZF);
> > +	      break;
> > +	    case 'c':
> > +	    case 'p':
> 
> I don't think "pf" should be recognized here. As terminology says here and in
> the spec, it's OSZC (no P in there).

> > +	    case 'a':
> > +	      break;
> 
> "af" pretty certainly may not be recognized here, as that would imply
> EFLAGS.AF becoming set, not cleared.
>

I know what you mean, SCC cannot test PF.

• If SCC = 0b1010, then SCC evaluates to true regardless of the status flags value.
• If SCC = 0b1011, then SCC evaluates to false regardless of the status flags value.
Consequently, the SCC cannot test the parity flag PF.

But they are listed in another place, and we should assign PF to EVEX.CF and no update for AF.

• If SCC evaluates to false on the status flags, then the CMP or TEST is not executed and instead the
status flags are updated as follows:
– OF = EVEX.OF
– SF = EVEX.SF
– ZF = EVEX.ZF
– CF = EVEX.CF
– PF = EVEX.CF
– AF = 0

> 
> > +	      i.oszc_flags |= (1 << CF);
> > +	      break;
> 
> Shouldn't you further reject redundant settings, as in
> 
> 	ccmpe {dfv=cf,cf} ...
> 
> ?

How about keeping this compatibility? Like any other pseudo prefix.

> > +	    default:
> > +	      as_bad (_("Unrecognized oszc flags"));
> > +	      ignore_rest_of_line ();
> > +	      return NULL;
> > +	    }
> > +	  op_string += 2;
> > +	}
> > +    }
> > +  return op_string;
> > +}
> > +
> >  /* Handle Vector operations.  */
> >
> >  static char *
> > @@ -14486,6 +14603,14 @@ i386_att_operand (char *operand_string)
> >        i.jumpabsolute = true;
> >      }
> >
> > +  /* Handle SCC OSZC flgs.  */
> > +  if (current_templates.start->opcode_modifier.scc)
> > +    {
> > +      op_string = check_Scc_OszcOperations (op_string);
> > +      if (op_string == NULL)
> > +	return 0;
> > +    }
> 
> See remark on i386_intel_operand().
> 

I will respond to the decoder's comments in another email, thanks.

Lili.