Re: [PATCH v3 4/9] Support APX GPR32 with extend evex prefix

[Jan Beulich <jbeulich@suse.com>]

On 13.12.2023 08:36, Cui, Lili wrote:
>>>>>>>>>>> @@ -3670,10 +3673,11 @@ install_template (const
>> insn_template
>>>>>>>>>>> *t)
>>>>>>>>>>>
>>>>>>>>>>>    /* Dual VEX/EVEX templates need stripping one of the
>>>>>>>>>>> possible
>>>>>>>> variants.  */
>>>>>>>>>>>    if (t->opcode_modifier.vex && t->opcode_modifier.evex)
>>>>>>>>>>> -  {
>>>>>>>>>>> -      if ((maybe_cpu (t, CpuAVX) || maybe_cpu (t, CpuAVX2)
>>>>>>>>>>> -	   || maybe_cpu (t, CpuFMA))
>>>>>>>>>>> -	  && (maybe_cpu (t, CpuAVX512F) || maybe_cpu (t,
>>>> CpuAVX512VL)))
>>>>>>>>>>> +    {
>>>>>>>>>>> +      if (AVX512F(CpuAVX) || AVX512F(CpuAVX2) ||
>>>> AVX512F(CpuFMA)
>>>>>>>>>>> +	  || AVX512VL(CpuAVX) || AVX512VL(CpuAVX2) ||
>>>>>>>>>> APX_F(CpuCMPCCXADD)
>>>>>>>>>>> +	  || APX_F(CpuAMX_TILE) || APX_F(CpuAVX512F) ||
>>>>>>>>>> APX_F(CpuAVX512DQ)
>>>>>>>>>>> +	  || APX_F(CpuAVX512BW) || APX_F(CpuBMI) ||
>>>>>> APX_F(CpuBMI2))
>>>>>>>>>>>  	{
>>>>>>>>>>>  	  if (need_evex_encoding ())
>>>>>>>>>>
>>>>>>>>>> There are several issues here:
>>>>>>>>>> - Why did you need to change (to the worse) the original code?
>>>>>>>>>> - Why did you not model the addition after that original code?
>>>>>>>>>> - How come APX_F (CpuAVX512*) constructs appear here, when no
>>>>>>>> AVX512
>>>>>>>>>> insn can be VEX-encoded?
>>>>>>>>>
>>>>>>>>>  I don't understand what you mean, we have this combination.
>>>>>>>>>
>>>>>>>>> kmov<dq>, 0x<dq:kpfx>90, AVX512BW&(AVX512BW|APX_F),
>>>>>>>>> Modrm|Vex128|EVex128|Space0F|VexW1|<dq:kvsz>|NoSuf, {
>>>>>>>>> RegMask|<dq:elem>|Unspecified|BaseIndex, RegMask }
>>>>>>>>
>>>>>>>> Oh, I'm sorry: I forgot about the mask register insns.
>>>>>>>>
>>>>>>>>>> - If these new macros are really needed for whatever reason,
>>>>>>>>>> they
>>>>>>>> shouldn't
>>>>>>>>>>   be added to opcodes/i386-opc.h when they're useful only in
>>>>>>>>>> the
>>>>>>>> assembler.
>>>>>>>>>> - Style requires a blank before the opening parenthesis in function
>>>>>>>>>>   invocations (which also covers function-like macro invocations).
>>>>>>>>>>
>>>>>>>>>> I think I asked before: How is it that you get away without
>>>>>>>>>> altering cpu_flags_match(), containing related and quite
>>>>>>>>>> similar
>>>> logic?
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>> For the original logic ( ... || ... ) && ( ... || ...), the
>>>>>>>>> content in the first bracket
>>>>>>>> and the content in the following brackets can be combined
>>>>>>>> arbitrarily. I think it is Inaccurate.
>>>>>>>>
>>>>>>>> In which way? If there are issues with the existing code, these
>>>>>>>> issues want taking care of in separate (prereq) patches. Of
>>>>>>>> course there are assumptions made here about the CPU combinations
>>>>>>>> that can (and cannot) occur in any of our templates. Similar
>>>>>>>> assumptions are imo
>>>>>> fine to make in the APX additions.
>>>>>>>>
>>>>>>>> Note how I used two nested if()s despite that not having been
>>>>>>>> necessary at that time. I did so in anticipation that for APX
>>>>>>>> you'd want to add another
>>>>>>>> (separate) inner if(), rather than altering the one that's there.
>>>>>>>
>>>>>>> Could we remove the CPU check here? it's a bit ugly and has
>>>>>>> limited
>>>>>> effectiveness.
>>>>>>>
>>>>>>>   if (t->opcode_modifier.vex && t->opcode_modifier.evex)
>>>>>>>     {
>>>>>>>       if (AVX512F(CpuAVX) || AVX512F(CpuAVX2) || AVX512F(CpuFMA)
>>>>>>>           || AVX512VL(CpuAVX) || AVX512VL(CpuAVX2) ||
>>>>>> APX_F(CpuCMPCCXADD)
>>>>>>>           || APX_F(CpuAMX_TILE) || APX_F(CpuAVX512F) ||
>>>>>> APX_F(CpuAVX512DQ)
>>>>>>>           || APX_F(CpuAVX512BW) || APX_F(CpuBMI) ||
>>>>>>> APX_F(CpuBMI2))
>>>>>>
>>>>>> I agree on the "a bit ugly" part, but taking what's there right now
>>>>>> I don't understand "has limited effectiveness". Of course you can
>>>>>> remove any code you want, provided you can prove nothing breaks.
>>>>>>
>>>>>
>>>>> Here is install_template().
>>>>> All I can say is that after removing the CPU check, no test cases
>>>>> failed. I
>>>> know it's hard to convince you to delete this place, or what do you
>>>> suggest to do with this? APX requires this, otherwise the test cases will fail.
>>>>>
>>>>> -      if (AVX512F(CpuAVX) || AVX512F(CpuAVX2) || AVX512F(CpuFMA)
>>>>> -         || AVX512VL(CpuAVX) || AVX512VL(CpuAVX2) ||
>>>> APX_F(CpuCMPCCXADD)
>>>>> -         || APX_F(CpuAMX_TILE) || APX_F(CpuAVX512F) ||
>>>> APX_F(CpuAVX512DQ)
>>>>> -         || APX_F(CpuAVX512BW) || APX_F(CpuBMI) || APX_F(CpuBMI2))
>>>>> -       {
>>>>
>>>> So be it then (assuming you don't delete any pre-existing code
>>>> there). As said, I expect this will bite us later.
>>>
>>> Done.
>>
>> I can't connect this with ...
>>
>>> +      if ((maybe_cpu (t, CpuAVX) || maybe_cpu (t, CpuAVX2)
>>> +          || maybe_cpu (t, CpuFMA))
>>> +         && (maybe_cpu (t, CpuAVX512F) || maybe_cpu (t, CpuAVX512VL))
>>> +         || APX_F(CpuCMPCCXADD) || APX_F(CpuAMX_TILE) ||
>> APX_F(CpuAVX512F)
>>> +         || APX_F(CpuAVX512DQ) || APX_F(CpuAVX512BW) ||
>> APX_F(CpuBMI)
>>> +         || APX_F(CpuBMI2))
>>
>> ... this: You said you want to remove all the new checks. And now you say
>> "done" with the checks all still there? And even if I misunderstood you, I still
>> don't see why you'd modify the existing condition: The adjustments made in
>> the body of the if() aren't applicable to APX afaict. Plus there are still the odd
>> APX_F() uses; I'm sure I commented on that before. If any adjustments need
>> making for APX, you want to add a 2nd inner if() inside the enclosing one.
>>
> 
> I want to remove all, including your pre-existing code,  there is an EVEX testcase failure due to not clean i.tm.opcode_modifier.vex = 0;  As you required that don't delete any pre-existing code, so I still need to add my new combination,  
> 
> How about this ?
> 
> 
> I want to remove all code, including your pre-existing code, VEX test case fails because it wasn't cleaned up i.tm.opcode_modifier.evex = 0; As you asked, don't remove any pre-existing code, so I still need to add my new combinations.
> 
> How about this?
> 
>   /* Dual VEX/EVEX templates need stripping one of the possible variants.  */
>   if (t->opcode_modifier.vex && t->opcode_modifier.evex)
>     {
>       if ((maybe_cpu (t, CpuAVX) || maybe_cpu (t, CpuAVX2)
>            || maybe_cpu (t, CpuFMA))
>           && (maybe_cpu (t, CpuAVX512F) || maybe_cpu (t, CpuAVX512VL)))
>         {
>           if (need_evex_encoding ())
>             {
>               i.tm.opcode_modifier.vex = 0;
>               i.tm.cpu.bitfield.cpuavx512f = i.tm.cpu_any.bitfield.cpuavx512f;
>               i.tm.cpu.bitfield.cpuavx512vl = i.tm.cpu_any.bitfield.cpuavx512vl;
>             }
>           else
>             {
>               i.tm.opcode_modifier.evex = 0;
>               if (i.tm.cpu_any.bitfield.cpuavx)
>                 i.tm.cpu.bitfield.cpuavx = 1;
>               else if (!i.tm.cpu.bitfield.isa)
>                 i.tm.cpu.bitfield.isa = i.tm.cpu_any.bitfield.isa;
>               else
>                 gas_assert (i.tm.cpu.bitfield.isa == i.tm.cpu_any.bitfield.isa);
>             }
>         }
> 
>       if (APX_F(CpuCMPCCXADD) || APX_F(CpuAMX_TILE) || APX_F(CpuAVX512F)
>           || APX_F(CpuAVX512DQ) || APX_F(CpuAVX512BW) || APX_F(CpuBMI)
>           || APX_F(CpuBMI2))
>         if (need_evex_encoding ())
>           i.tm.opcode_modifier.vex = 0;
>         else
>           i.tm.opcode_modifier.evex = 0;
>     }

Something along these lines, indeed. But without APX_F(). I've just looked
it up again:

#define APX_F(cpuid) (maybe_cpu (t, CpuAPX_F) && maybe_cpu (t, cpuid))

Why would you test CpuAPX_F over and over again in the conditional? See
how the code that has been there for a little while checks each CpuXYZ
exactly once.

Plus, simply as a style remark, you want to add braces around the if/else,
to make entirely clear that the else belongs to the inner if() (iirc some
compiler versions warn about code as you have it above).

Jan