RE: [PATCH 1/3] Support APX CCMP and CTEST

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

> On 11.06.2024 10:06, Cui, Lili wrote:
> > @@ -1929,6 +1939,114 @@ 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 bool set_oszc_flags (unsigned int oszc_shift) {
> > +  if (i.oszc_flags & oszc_shift)
> > +    {
> > +      as_bad (_("same oszc flag used twice"));
> > +      return false;
> > +    }
> > +  i.oszc_flags |= oszc_shift;
> > +  return true;
> > +}
> > +
> > +/* Handle SCC OSZC flags.  */
> > +
> > +static int
> > +check_Scc_OszcOperations (const char *l) {
> > +  const char *suffix_string = l;
> > +  bool has_dfv = false;
> > +
> > +  while (is_space_char (*suffix_string))
> > +    suffix_string++;
> > +
> > +  /* If {oszc flags} is absent, just return.  */  if (*suffix_string
> > + != '{')
> > +    return 0;
> > +  else
> > +    suffix_string++;
> 
> Just to mention it: I'm pretty strongly against using "else" in cases like this
> one: It's more code, hence - even if just slightly - harder to read, for no gain at
> all. If you keep it like that, I may subsequently go through and purge all of
> those.
> 

Dropped 'else' here.

> > +  /* Parse 'dfv='.  */
> > +  while (*suffix_string)
> > +    {
> > +      if (is_space_char (*suffix_string))
> > +	suffix_string++;
> > +      else if (*suffix_string == '=')
> > +	{
> > +	  suffix_string++;
> > +	  break;
> > +	}
> > +      else if (startswith (suffix_string, "dfv") && !has_dfv)
> > +	{
> > +	  suffix_string += 3;
> > +	  has_dfv = true;
> > +	}
> > +      else
> > +	{
> > +	  as_bad (_("Unrecognized pseudo-suffix"));
> > +	  return -1;
> > +	}
> > +    }
> 
> Hmm, a pretty firm expectation of mine was that this now wouldn't be done
> as a loop anymore. It's not strictly necessary to change, yet it looks as if this
> code structure wouldn't lend itself to there appearing another pseudo-suffix,
> which then also would want recognizing here.
> 
My initial thought was that using a loop would make it easier to get rid of the extra spaces. If the loop is removed, the code becomes as follows, the space removal operation needs to be repeated. 

  while (is_space_char (*suffix_string))
    suffix_string++;

  if (strcasecmp (suffix_string, "dfv") > 0)
    suffix_string += 3;
 else
  as_bad (_("Unrecognized pseudo-suffix"));

  while (is_space_char (*suffix_string))
    suffix_string++;

  if (*suffix_string == '=')
    suffix_string++;
 else
    as_bad (_("Unrecognized pseudo-suffix"));


> > +  /* Parse 'of , sf, zf, cf}'.  */
> > +  while (*suffix_string)
> > +    {
> > +      if (*suffix_string == ',' || is_space_char (*suffix_string))
> > +	suffix_string++;
> 
> Like for the earlier loop in the earlier version: Is it really okay to have multiple
> successive commas (with or without whitespace in between)?
> 

Ok, I'll add a check for it.

> > +      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':
> > +	      if (set_oszc_flags (OSZC_OF))
> > +		break;
> > +	      else
> > +		return -1;
> > +	    case 's':
> > +	      if (set_oszc_flags (OSZC_SF))
> > +		break;
> > +	      else
> > +		return -1;
> > +	    case 'z':
> > +	      if (set_oszc_flags (OSZC_ZF))
> > +		break;
> > +	      else
> > +		return -1;
> > +	    case 'c':
> > +	      if (set_oszc_flags (OSZC_CF))
> > +		break;
> > +	      else
> > +		return -1;
> > +	    default:
> > +	      as_bad (_("Unrecognized oszc flags"));
> > +	      return -1;
> > +	    }
> 
> Hmm, again, I was kind of expecting this to be done differently, such that both
> kinds of errors would be flagged if both are present in a single insn.
> But yes, the way you've done it should also be okay.
> 
> Separate aspect: Do we really want this pseudo-suffix to be case-sensitive
> when mnemonic parsing is all case-insensitive?
> 

Done.

> > +	  suffix_string += 2;
> > +	}
> > +    }
> > +
> > +  as_bad (_("Unbalanced `}' in suffix"));
> 
> Would you mind saying "pseudo-suffix" here?
> 

Your suggestion is more accurate.

> > @@ -7453,6 +7592,15 @@ parse_insn (const char *line, char *mnemonic,
> bool prefix_only)
> >  	    }
> >  	}
> >      }
> > +
> > +  /* Handle SCC OSZC flgs.  */
> > +  if (current_templates.start->opcode_modifier.operandconstraint == SCC)
> > +    {
> > +      int length = check_Scc_OszcOperations (l);
> > +      if (length < 0)
> > +	return NULL;
> > +      l += length;
> > +    }
> >    /* Any other comma loses.  */
> >    if (*l == ',')
> >      {
> 
> Looking again, I'm not sure this is a good insertion point. The comment about
> other commas here is associated with the earlier if(). These two blocks would
> better remain together. Moving yours ahead and make that earlier if() and
> else-if would seem most logical to me.
> 

Done.

> > @@ -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.  */
> > +	      oappend (ins, scc_suffix[ins->vex.scc]);
> > +
> > +	      /* For SCC insns, the ND bit is required to be set to 0.  */
> > +	      if (ins->vex.nd)
> > +		{
> > +		  oappend (ins, "(bad)");
> > +		  break;
> 
> Btw, I'd like to suggest to drop this "break", such that nevertheless ...
> 
> > +		}
> > +
> > +	      /* These bits have been consumed and should be cleared or
> restored
> > +		 to default values.  */
> > +	      ins->vex.nf = false;
> > +	      ins->vex.mask_register_specifier = 0;
> 
> ... this cleanup is being done (to avoid yet more badness printing elsewhere).
> 

Agree, this is better.

> > @@ -10637,6 +10692,19 @@ putop (instr_info *ins, const char
> *in_template, int sizeflag)
> >  		  evex_printed = true;
> >  		}
> >  	    }
> > +	  else if (l == 1 && last[0] == 'D')
> > +	    {
> > +	      /* Get oszc flags value from register_specifier.  */
> > +	      int oszc_value = ~ins->vex.register_specifier & 0xf;
> > +
> > +	      /* Add {dfv=of, sf, zf, cf} flags.  */
> > +	      oappend (ins, oszc_flags[oszc_value]);
> > +
> > +	      /* These bits have been consumed and should be cleared or
> restored
> > +		 to default values.  */
> > +	      ins->vex.v = 1;
> > +	      ins->vex.register_specifier = 0;
> 
> vex.register_specifier was consumed here, yes, but vex.v belongs to SCC
> handling, doesn't it?
> 

Oh! yes, vex.v and vex.nf share the same bit.

Thanks,
Lili.