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

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

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

I found a similar situation in this function and made changes, thanks for the reminder.

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

I prefer the loop one.

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

I don't understand the logic of your approach, I think I missed some information. Currently I am using the following method to implement it.

bool check_comma = true;

  while (*suffix_string)
    {
      if (*suffix_string == ',')
        {
          /* Report an error for illegal commas.  */
          if (check_comma == true)
            {
              as_bad (_("Illegal comma found in pseudo-suffix"));
              return -1;
            }
          check_comma = true;
          suffix_string++;
        }
      else if (is_space_char (*suffix_string))
        suffix_string++;
      else if (*suffix_string == '}')
        {
          suffix_string++;
          return suffix_string - l;
        }
      else
        {
          check_comma = false;
          ...
          suffix_string += 2;
        }
    }

> >>> @@ -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).
> 
Yes, you are right.

Thanks,
Lili.