RE: [PATCH] x86: add missing APX logic to cpu_flags_match()

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

> On 08.01.2024 09:30, Cui, Lili wrote:
> >>>> --- a/gas/config/tc-i386.c
> >>>> +++ b/gas/config/tc-i386.c
> >>>> @@ -1940,6 +1940,30 @@ cpu_flags_match (const insn_template *t)
> >>>>  	      any.bitfield.cpuavx512vl = 0;
> >>>>  	    }
> >>>>  	}
> >>>> +
> >>>> +      /* Dual non-APX/APX templates need massaging from what
> >>>> + APX_F() in
> >> the
> >>>> +         opcode table has produced.  While the direct transformation of the
> >>>> +         incoming cpuid&(cpuid|APX_F) would be to cpuid&(cpuid) /
> >>>> cpuid&(APX_F)
> >>>> +         respectively, it's cheaper to move to just cpuid / cpuid&APX_F
> >>>> +         instead.  */
> >>>> +      if (any.bitfield.cpuapx_f
> >>>> +	  && (any.bitfield.cpubmi || any.bitfield.cpubmi2
> >>>> +	      || any.bitfield.cpuavx512f || any.bitfield.cpuavx512bw
> >>>> +	      || any.bitfield.cpuavx512dq || any.bitfield.cpuamx_tile
> >>>> +	      || any.bitfield.cpucmpccxadd))
> >>>> +	{
> >>>> +	  /* These checks (verifying that APX_F() was properly used in the
> >>>> +	     opcode table entry) make sure there's no need for an "else" to
> >>>> +	     the "if()" below.  */
> >>>> +	  gas_assert (!cpu_flags_all_zero (&all));
> >>>> +	  cpu = cpu_flags_and (all, any);
> >>>> +	  gas_assert (cpu_flags_equal (&cpu, &all));
> >>>> +
> >>>> +	  if (need_evex_encoding (t))
> >>>> +	    all = any;
> >>>> +
> >>>
> >>>> +	  memset (&any, 0, sizeof (any));
> >>>
> >>> Wouldn't it make sense to put it in the else branch and clean out
> >>> APX-F
> >> specifically? Just like you did before.
> >>>
> >>>   if (need_evex_encoding (t))
> >>>     all = any;
> >>> else
> >>>    any.bitfield.cpuapx_f = 0;
> >>
> >> That was an alternative I did consider, yes, but the way I've done it
> >> is overall more self-consistent imo, at the expense of being less
> >> consistent with the
> >> AVX/AVX512 logic (the moving of "any" to "all" isn't consistent with
> >> that anyway).
> >>
> >
> >   memset (&any, 0, sizeof (any));
> >
> > I'd say this would make "any" not match the actual value, which might be
> used later, but it's been cleared here.
> 
> I'm afraid I don't get what you're trying to tell me.
> 

What I mean is that memset will clear the variable "any", there is no problem in handling it this way. But I think the following way is more reasonable.

For evex it should be:
any.bitfield.cpubmi = 1
any.bitfield.cpuapx_f = 1

For vex it should be:
any.bitfield.cpubmi = 1
any.bitfield.cpuapx_f = 0

Instead of clearing all values in "any".

Lili.