Re: [PATCH 1/3] x86: Split REX/REX2 old registers judgment.

[Jan Beulich <jbeulich@suse.com>]

On 22.05.2024 08:11, Cui, Lili wrote:
>> On 22.05.2024 03:33, Cui, Lili wrote:
>>>> On 20.05.2024 08:22, Cui, Lili wrote:
>>>>> --- a/gas/config/tc-i386.c
>>>>> +++ b/gas/config/tc-i386.c
>>>>> @@ -4303,22 +4303,20 @@ static void establish_rex (void)
>>>>>    /* Respect a user-specified REX prefix.  */
>>>>>    i.rex |= i.prefix[REX_PREFIX] & REX_OPCODE;
>>>>>
>>>>> -  /* For 8 bit registers we need an empty rex prefix.  Also if the
>>>>> -     instruction already has a prefix, we need to convert old
>>>>> -     registers to new ones.  */
>>>>> -
>>>>> -  if ((i.types[first].bitfield.class == Reg && i.types[first].bitfield.byte
>>>>> -       && ((i.op[first].regs->reg_flags & RegRex64) != 0 || i.rex != 0
>>>>> -	   || i.rex2 != 0))
>>>>> -      || (i.types[last].bitfield.class == Reg && i.types[last].bitfield.byte
>>>>> -	  && ((i.op[last].regs->reg_flags & RegRex64) != 0 || i.rex != 0
>>>>> -	      || i.rex2 != 0)))
>>>>> -    {
>>>>> -      unsigned int x;
>>>>> -
>>>>> -      if (!is_apx_rex2_encoding () && !is_any_vex_encoding(&i.tm))
>>>>> -	i.rex |= REX_OPCODE;
>>>>> -      for (x = first; x <= last; x++)
>>>>> +  /* For 8 bit registers without a prefix, we need an empty rex
>>>>> + prefix.  */
>>>>
>>>> As you're touching this comment, can you please also correct it? It's
>>>> not all 8- bit registers which are affected here (as expressed by the
>>>> RegRex64 checks).
>>>>
>>> Done.
>>>
>>>>> +  if (((i.types[first].bitfield.class == Reg && i.types[first].bitfield.byte
>>>>> +	&& ((i.op[first].regs->reg_flags & RegRex64) != 0 ))
>>>>
>>>> Nit: Please parenthesize this line and ...
>>>>
>>>>> +       || (i.types[last].bitfield.class == Reg && i.types[last].bitfield.byte
>>>>> +	   && (i.op[last].regs->reg_flags & RegRex64) != 0 ))
>>>>
>>>> ... this one the same way, preferably like the latter. Also please
>>>> omit the blanks before the closing parentheses.
>>>>
>>> Done.
>>>
>>>>> +      && !is_apx_rex2_encoding () && !is_any_vex_encoding (&i.tm)
>>>>> + &&
>>>>> + !i.rex)
>>>>
>>>> Is the !i.rex part needed? It wasn't there before. If it's needed, it
>>>> would imo better come ahead of the APX/VEX checks.
>>>>
>>> Yes, I added it because there is already i.rex and we don't need to overwrite
>> it. And I will put it ahead of the APX/VEX checks.
>>
>> I don't think conditionals should be added in such cases. Resulting code will do
>> better without the extra conditional branch, when the update is really benign
>> in the case where (here) i.rex is already set. See, btw, your own response to a
>> pretty similar question of mine:
>> https://sourceware.org/pipermail/binutils/2024-May/134157.html
>>
> Yes, they are the similar issues. Since I just pushed it to the trunk, I'll create a patch to remove it.

Thanks, and pre-approved.

Jan