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

[Jan Beulich <jbeulich@suse.com>]

On 08.01.2024 11:28, Cui, Lili wrote:
>> 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".

But why would you want to have the same in "any" that you already have in
"all"? That would incur extra checks later in the function for no gain.

Jan