Re: [PATCH] x86: Optimize the encoder of the vvvv register

[Jan Beulich <jbeulich@suse.com>]

On 22.04.2024 10:42, Cui, Lili wrote:
>> On 19.04.2024 09:36, Cui, Lili wrote:
>>> @@ -1000,13 +1002,13 @@ pause, 0xf390, i186, NoSuf, {}
>>>
>>>  // MMX/SSE2 instructions.
>>>
>>> -<mmx:cpu:pfx:attr:reg:mem, +
>>> -    $avx:AVX:66:Vex128|VexVVVV|VexW0|SSE2AVX:RegXMM:Xmmword, +
>>> -    $sse:SSE2:66::RegXMM:Xmmword, +
>>> -    $mmx:MMX:::RegMMX:Qword>
>>> +<mmx:cpu:pfx:attr:scal:reg:mem, +
>>> +
>> $avx:AVX:66:Vex128|VexVVVV_Src1|VexW0|SSE2AVX:Vex128|VexVVVV_Dst
>> |VexW0|SSE2AVX:RegXMM:Xmmword, +
>>> +    $sse:SSE2:66:::RegXMM:Xmmword, +
>>> +    $mmx:MMX::::RegMMX:Qword>
>>>
>>>  <sse2:cpu:attr:scal:vvvv, +
>>> -    $avx:AVX:Vex128|VexW0|SSE2AVX:VexLIG|VexW0|SSE2AVX:VexVVVV, +
>>> +
>> $avx:AVX:Vex128|VexW0|SSE2AVX:VexLIG|VexW0|SSE2AVX:VexVVVV_Src1,
>>> + +
>>>      $sse:SSE2:::>
>>>
>>>  <bw:opc:vexw:elem:kcpu:kpfx:cpubmi, + @@ -1058,7 +1060,7 @@
>>> pmulhw<mmx>, 0x<mmx:pfx>0fe5, <mmx:cpu>,
>> Modrm|<mmx:attr>|C|NoSuf, {
>>> <mmx:reg>|<  pmullw<mmx>, 0x<mmx:pfx>0fd5, <mmx:cpu>,
>>> Modrm|<mmx:attr>|C|NoSuf,
>> { <mmx:reg>|<mmx:mem>|Unspecified|BaseIndex,
>>> <mmx:reg> }  por<mmx>, 0x<mmx:pfx>0feb, <mmx:cpu>,
>>> Modrm|<mmx:attr>|C|NoSuf,
>> { <mmx:reg>|<mmx:mem>|Unspecified|BaseIndex,
>>> <mmx:reg> }  psllw<mmx>, 0x<mmx:pfx>0ff1, <mmx:cpu>,
>>> Modrm|<mmx:attr>|NoSuf,
>> { <mmx:reg>|<mmx:mem>|Unspecified|BaseIndex,
>>> <mmx:reg> } -psllw<mmx>, 0x<mmx:pfx>0f71/6, <mmx:cpu>,
>>> Modrm|<mmx:attr>|NoSuf, { Imm8, <mmx:reg> }
>>> +psllw<mmx>, 0x<mmx:pfx>0f71/6, <mmx:cpu>,
>> Modrm|<mmx:scal>|NoSuf, {
>>> +Imm8, <mmx:reg> }
>>
>> This is not a scalar instruction, hence "scal" as a template parameter name is
>> misleading. It's not really clear to me anyway why this needs fiddling with -
>> there was no SwapSources here, and none of its siblings are being touched
>> either.
>>
> 
> 'psllw' has an extended opcode and two non-immediate operands, to delete the corresponding code below.
> 
> -  if (i.tm.extension_opcode != None)
> -    {
> -      if (dest != source)
> -       v = dest;
> -      dest = ~0;
> -    }

Yet how's psllw different from, say, psraw or pslld?

>> To help recognizing such anomalies (possible problems), could I talk you into
>> splitting the patch in two pieces? First a purely mechanical one introducing
>> (perhaps simply as an alias of VexVVVV) / using Src1VVVV wherever it is
>> meant to be used. Then the remaining changes, with a much smaller diff on
>> the actual opcode templates, in the 2nd one.
>>
> 
> How about splitting into 3 patches:
> 1. Introduce VexVVVV , Src1VVVV and Src2VVVV. 
> 2. Replace SwapSources. 
> 3. Replace extension_opcode part.

Fundamentally fine with me, just that it would seem to me that in such a 1st
patch Src2VVVV would end up unused. Hence it would appear more logical to me
to introduce that only when needed, i.e. in patch 2.

Jan