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

[Jan Beulich <jbeulich@suse.com>]

On 08.01.2024 04:17, Cui, Lili wrote:
>> As already indicated during review, we can't get away without certain
>> adjustments here: Without these, respective {evex}-prefixed insns are
>> assembled to APX encodings even when APX_F is turned off.
>>
>> While there also extend the respective comment in the opcode table, to
>> explain why this construct is used.
>> ---
>> Strictly speaking we could go with just cpuid|APX_F in the templates, with the
>> assertions dropped and instead an "else" added. But I think we're better off
>> this way, for being less prone to introducing mistakes later on.
>>
>> The resulting diagnostics aren't quite correct (because of not mentioning the
>> {evex} prefix, which really is what's the problem there), but improving
>> diagnostics is a wider topic anyway.
> 
> Oh, get it, thanks.
> Currently, a lot of ugly special handling has been added to i386 in order to merge these vex/evex insns, which increases the complexity of the code. Personally, I don't think it's worth it. Other issues may arise later.

Well, if it was just for the saving of a few templates, it probably indeed
wouldn't be worth it. But the set of templates saved is quite significant
overall, hence why I did it in the first place. The number of templates to
consult does, after all, matter when processing a respective insn.

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

Jan