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

  Split "REX/REX2 old register checking" and "add empty rex prefix"
into two separate branches.

gas/ChangeLog:

        * config/tc-i386.c (establish_rex): Split the judgments.
---
 gas/config/tc-i386.c | 30 ++++++++++++++----------------
 1 file changed, 14 insertions(+), 16 deletions(-)
  

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).

> +  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.

> +      && !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.

> +    i.rex |= REX_OPCODE;
> +
> +  /* For REX/REX2 prefix instructions, we need to convert old registers
> +     (AL, CL, DL and BL) to new ones (AXL, CXL, DXL and BXL) and report bad
> +     for AH, CH, DH and BH.  */

"report bad for" is a little odd; how about simply "reject"?

Okay with respective adjustments.

Jan

> +  if (i.rex || i.rex2)
> +    {
> +      for (unsigned int x = first; x <= last; x++)
>  	{
>  	  /* Look for 8 bit operand that uses old registers.  */
>  	  if (i.types[x].bitfield.class == Reg && i.types[x].bitfield.byte
  

> 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.rex |= REX_OPCODE;
> > +
> > +  /* For REX/REX2 prefix instructions, we need to convert old registers
> > +     (AL, CL, DL and BL) to new ones (AXL, CXL, DXL and BXL) and report bad
> > +     for AH, CH, DH and BH.  */
> 
> "report bad for" is a little odd; how about simply "reject"?
> 
Done.

> Okay with respective adjustments.

Thanks,
Lili.
  

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

Jan
  

> 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.

Lili.
  

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