diff mbox series

[1/5] x86: correct {,V}PEXTR{D,Q} optimization

Message ID d13be8b8-d501-4a19-a359-813fd9f2e57e@suse.com
State New
Headers
Series x86: (mainly) insert/extract optimizations |

Checks

Context Check Description
linaro-tcwg-bot/tcwg_binutils_build--master-arm fail Patch failed to apply
linaro-tcwg-bot/tcwg_binutils_build--master-aarch64 fail Patch failed to apply

Commit Message

Jan Beulich Sept. 6, 2024, 11:51 a.m. UTC 
  A possible relocation associated with a memory operand also needs
moving.

Comments

Cui, Lili Sept. 11, 2024, 7:07 a.m. UTC | #1 
> -----Original Message-----
> From: Jan Beulich <jbeulich@suse.com>
> Sent: Friday, September 6, 2024 7:52 PM
> To: Binutils <binutils@sourceware.org>
> Cc: H.J. Lu <hjl.tools@gmail.com>
> Subject: [PATCH 1/5] x86: correct {,V}PEXTR{D,Q} optimization
> 
> A possible relocation associated with a memory operand also needs moving.
> 
> --- a/gas/config/tc-i386.c
> +++ b/gas/config/tc-i386.c
> @@ -5503,6 +5503,7 @@ optimize_encoding (void)
>        i.op[1].regs = i.op[2].regs;
>        i.types[1] = i.types[2];
>        i.flags[1] = i.flags[2];
> +      i.reloc[1] = i.reloc[2];


      /* Optimize: -O:
         pextrd $0, %xmmN, ...   -> movd %xmmN, ...
         pextrq $0, %xmmN, ...   -> movq %xmmN, ...
         vpextrd $0, %xmmN, ...  -> vmovd %xmmN, ...
         vpextrq $0, %xmmN, ...  -> vmovq %xmmN, ...
       */

Jan, I have a question, why do you want to add the swap of i.reloc[] here (seems they are not related to reloc)? There is no swapping of i.reloc[] elsewhere in this function.

Thanks,
Lili.

>        i.tm.operand_types[1] = i.tm.operand_types[2];
> 
>        i.operands = 2;
Jan Beulich Sept. 11, 2024, 7:19 a.m. UTC | #2 
On 11.09.2024 09:07, Cui, Lili wrote:
>> -----Original Message-----
>> From: Jan Beulich <jbeulich@suse.com>
>> Sent: Friday, September 6, 2024 7:52 PM
>>
>> --- a/gas/config/tc-i386.c
>> +++ b/gas/config/tc-i386.c
>> @@ -5503,6 +5503,7 @@ optimize_encoding (void)
>>        i.op[1].regs = i.op[2].regs;
>>        i.types[1] = i.types[2];
>>        i.flags[1] = i.flags[2];
>> +      i.reloc[1] = i.reloc[2];
> 
> 
>       /* Optimize: -O:
>          pextrd $0, %xmmN, ...   -> movd %xmmN, ...
>          pextrq $0, %xmmN, ...   -> movq %xmmN, ...
>          vpextrd $0, %xmmN, ...  -> vmovd %xmmN, ...
>          vpextrq $0, %xmmN, ...  -> vmovq %xmmN, ...
>        */
> 
> Jan, I have a question, why do you want to add the swap of i.reloc[] here (seems they are not related to reloc)? There is no swapping of i.reloc[] elsewhere in this function.

In most cases of what is being optimized there are no memory operands, or
the number of operands total doesn't change. That's different here. And
memory operands can come with a relocation, which needs to be kept
associated with the correct operands.

Jan
Cui, Lili Sept. 11, 2024, 7:41 a.m. UTC | #3 
> -----Original Message-----
> From: Jan Beulich <jbeulich@suse.com>
> Sent: Wednesday, September 11, 2024 3:20 PM
> To: Cui, Lili <lili.cui@intel.com>
> Cc: H.J. Lu <hjl.tools@gmail.com>; Binutils <binutils@sourceware.org>
> Subject: Re: [PATCH 1/5] x86: correct {,V}PEXTR{D,Q} optimization
> 
> On 11.09.2024 09:07, Cui, Lili wrote:
> >> -----Original Message-----
> >> From: Jan Beulich <jbeulich@suse.com>
> >> Sent: Friday, September 6, 2024 7:52 PM
> >>
> >> --- a/gas/config/tc-i386.c
> >> +++ b/gas/config/tc-i386.c
> >> @@ -5503,6 +5503,7 @@ optimize_encoding (void)
> >>        i.op[1].regs = i.op[2].regs;
> >>        i.types[1] = i.types[2];
> >>        i.flags[1] = i.flags[2];
> >> +      i.reloc[1] = i.reloc[2];
> >
> >
> >       /* Optimize: -O:
> >          pextrd $0, %xmmN, ...   -> movd %xmmN, ...
> >          pextrq $0, %xmmN, ...   -> movq %xmmN, ...
> >          vpextrd $0, %xmmN, ...  -> vmovd %xmmN, ...
> >          vpextrq $0, %xmmN, ...  -> vmovq %xmmN, ...
> >        */
> >
> > Jan, I have a question, why do you want to add the swap of i.reloc[] here
> (seems they are not related to reloc)? There is no swapping of i.reloc[] elsewhere
> in this function.
> 
> In most cases of what is being optimized there are no memory operands, or the
> number of operands total doesn't change. That's different here. And memory
> operands can come with a relocation, which needs to be kept associated with
> the correct operands.
> 
Yes, it is safer.

Thanks,
Lili.
> Jan
diff mbox series

Patch

--- a/gas/config/tc-i386.c
+++ b/gas/config/tc-i386.c
@@ -5503,6 +5503,7 @@  optimize_encoding (void)
       i.op[1].regs = i.op[2].regs;
       i.types[1] = i.types[2];
       i.flags[1] = i.flags[2];
+      i.reloc[1] = i.reloc[2];
       i.tm.operand_types[1] = i.tm.operand_types[2];
 
       i.operands = 2;