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

[Jan Beulich <jbeulich@suse.com>]

On 12.12.2023 14:15, Cui, Lili wrote:
>> On 12.12.2023 13:32, 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.

Jan