[PATCH 1/3] Support APX CCMP and CTEST

[Jan Beulich]

On 13.06.2024 12:30, Cui, Lili wrote:
>> 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.

Thanks. You realize though that I used this as example; there were several
more similar uses of "else" in the patch.

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

First: Whitespace removal doesn't need loops, if other code is to be
trusted. The scrubber collapses multiple of them into a single one anyway.
Second: The as_bad() here want following by bailing from the function.
Third: As indicated, I won't insist on you switching away from the loop
you had. I merely think that the alternative is better both from a source
clarity perspective and for resulting runtime behavior.

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

It's not really another check that's needed. When put at the bottom of the
loop body, your expectation simply is to find a brace or a comma. Anything
else is an error. (That way "{dfv=,cf}" would then also be properly
rejected.)

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

They don't, do they? EVEX.NF aliases EVEX.SC2, while EVEX.V4 aliases EVEX.SC3
afaics. The two belong together, though (as said in the earlier reply).

Jan