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

[Jan Beulich <jbeulich@suse.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.

> +  /* 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.

> +  /* 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)?

> +      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?

> +	  suffix_string += 2;
> +	}
> +    }
> +
> +  as_bad (_("Unbalanced `}' in suffix"));

Would you mind saying "pseudo-suffix" here?

> @@ -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.

> @@ -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).

> @@ -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?

Jan