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

[Jan Beulich <jbeulich@suse.com>]

On 26.04.2024 07:33, 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".
> 
> Do you want to split this patch 1/3 into two? Or move it to patch 2/3?

No. I'm asking for the title to be adjusted to state what is changing,
rather than what has already been the case forever since the introduction
of the struct field. This may be as simple as inserting "more directly".

Jan