Re: [PATCH 1/3] x86: Use vexvvvv to encode the vvvv register

[Jan Beulich <jbeulich@suse.com>]

On 25.04.2024 15:14, Cui, Lili wrote:
>> On 24.04.2024 09:23, Cui, Lili wrote:
>>> --- a/gas/config/tc-i386.c
>>> +++ b/gas/config/tc-i386.c
>>> @@ -5045,7 +5045,7 @@ optimize_encoding (void)
>>>         */
>>>        i.tm.opcode_space = SPACE_0F;
>>>        i.tm.base_opcode = 0x6c;
>>> -      i.tm.opcode_modifier.vexvvvv = 1;
>>> +      i.tm.opcode_modifier.vexvvvv = VexVVVV_SRC1;
>>>
>>>        ++i.operands;
>>>        ++i.reg_operands;
>>> @@ -10432,19 +10432,19 @@ build_modrm_byte (void)
>>>  				     || i.encoding == encoding_evex));
>>>      }
>>>
>>> -  if (i.tm.opcode_modifier.vexvvvv == VexVVVV_DST)
>>> +  switch (i.tm.opcode_modifier.vexvvvv)
>>>      {
>>> -      v = dest;
>>> -      dest-- ;
>>> -    }
>>> -  else
>>> -    {
>>> -      for (v = source + 1; v < dest; ++v)
>>> -	if (v != reg_slot)
>>> -	  break;
>>> -      if (v >= dest)
>>> -	v = ~0;
>>
>> Replacing this by ...
>>
>>> -    }
>>> +    case VexVVVV_SRC1:
>>> +      v =  dest - 1;
>>> +      break;
>>
>> ... just this could do with a word of explanation in the (sadly once again empty)
>> description. While I'm sure this tests out okay for you, it's not immediately clear
>> why the loop can be replaced this easily. Whereas by the end of the series (with
>> SwapSources dropped) this simplification is pretty obvious.
>>
> 
> SDM said "VEX.vvvv encodes the first source register operand". It should be "dest -1". The old logic did not check vexvvvv first, which made the logic here very complicated. I'll add some comments here.
> 
>> As to the deecription once again being empty: This is even more of an issue
>> considering that the title suggests the patch isn't doing anything.
>> After all we already use vexvvvv for the stated purpose.
> 
> The title is "Use vexvvvv to encode the vvvv register", which means that use vexvvvv for the stated purpose.

Well, yes and no: We did so before already, so the title suggests "no
change".

>>>  vpgatherqq, 0x6691, AVX2, Modrm|Vex256|Space0F38|Src1VVVV|VexW1|SwapSources|NoSuf|VecSIB2
>> 56, { RegYMM, Qword|Unspecified|BaseIndex, RegYMM }
>>
>> Anything using SwapSources wants leaving alone in this patch. There's no point
>> touching any of these twice (here and when subsequently you replace
>> SwapSources).
>> That'll also help to limit patch size a little.
>>
> 
> If we keep the old value VexVVVV here, we still need to deal with it in tc-i386.c and i386-opc.h, which is a bit strange.

VexVVVV without a value is the same as Src1VVVV, hence I don't think tc-i386.c
would be affected by leaving those in place.

Jan