Re: [PATCH][AArch64] PR target/71112: Properly create lowpart of pic_offset_table_rtx with -fpie

2016-11-30 Thread Kyrill Tkachov 


On 30/11/16 05:27, Andrew Pinski wrote:

On Tue, Nov 29, 2016 at 1:09 AM, Kyrill Tkachov
 wrote:

Hi all,

This ICE only occurs on big-endian ILP32 -fpie code. The expansion code
generates the invalid load:
(insn 6 5 7 (set (reg/f:SI 76)
 (unspec:SI [
 (mem/u/c:SI (lo_sum:SI (nil)
 (symbol_ref:SI ("dbs") [flags 0x40] )) [0  S4 A8])
 ] UNSPEC_GOTSMALLPIC28K))
  (expr_list:REG_EQUAL (symbol_ref:SI ("dbs") [flags 0x40] )
 (nil)))

to load the symbol. Note the (nil) argument to lo_sum.
The buggy hunk meant to take the lowpart of the pic_offset_table_rtx
register but it did so by explicitly
constructing a subreg, for which the offset is wrong for big-endian. The
right way is to use gen_lowpart which
knows what exactly to do, with this patch we emit:
(insn 6 5 7 (set (reg/f:SI 76)
 (unspec:SI [
 (mem/u/c:SI (lo_sum:SI (subreg:SI (reg:DI 73) 4)
 (symbol_ref:SI ("dbs") [flags 0x40] )) [0  S4 A8])
 ] UNSPEC_GOTSMALLPIC28K))
  (expr_list:REG_EQUAL (symbol_ref:SI ("dbs") [flags 0x40] )
 (nil)))

and everything works fine.

Bootstrapped and tested on aarch64-none-linux-gnu.
Also tested on aarch64_be-none-elf.
Ok for trunk?

Naveen posted the exact same patch:
https://gcc.gnu.org/ml/gcc-patches/2016-11/msg02305.html
Though with a slightly different testcase :).


Ah, indeed. Sorry for the noise :)
Kyrill


Thanks,
Andrew


Thanks,
Kyrill

2016-11-29  Kyrylo Tkachov  

 PR target/71112
 * config/aarch64/aarch64.c (aarch64_load_symref_appropriately,
 case SYMBOL_SMALL_GOT_28K): Use gen_lowpart rather than constructing
 subreg directly.

2016-11-29  Kyrylo Tkachov  

 PR target/71112
 * gcc.c-torture/compile/pr71112.c: New test.

Re: [PATCH][AArch64] PR target/71112: Properly create lowpart of pic_offset_table_rtx with -fpie

2016-11-29 Thread Andrew Pinski 
On Tue, Nov 29, 2016 at 1:09 AM, Kyrill Tkachov
 wrote:
> Hi all,
>
> This ICE only occurs on big-endian ILP32 -fpie code. The expansion code
> generates the invalid load:
> (insn 6 5 7 (set (reg/f:SI 76)
> (unspec:SI [
> (mem/u/c:SI (lo_sum:SI (nil)
> (symbol_ref:SI ("dbs") [flags 0x40]  0x7f6e387c0ab0 dbs>)) [0  S4 A8])
> ] UNSPEC_GOTSMALLPIC28K))
>  (expr_list:REG_EQUAL (symbol_ref:SI ("dbs") [flags 0x40]  0x7f6e387c0ab0 dbs>)
> (nil)))
>
> to load the symbol. Note the (nil) argument to lo_sum.
> The buggy hunk meant to take the lowpart of the pic_offset_table_rtx
> register but it did so by explicitly
> constructing a subreg, for which the offset is wrong for big-endian. The
> right way is to use gen_lowpart which
> knows what exactly to do, with this patch we emit:
> (insn 6 5 7 (set (reg/f:SI 76)
> (unspec:SI [
> (mem/u/c:SI (lo_sum:SI (subreg:SI (reg:DI 73) 4)
> (symbol_ref:SI ("dbs") [flags 0x40]  0x7ffb097e6ab0 dbs>)) [0  S4 A8])
> ] UNSPEC_GOTSMALLPIC28K))
>  (expr_list:REG_EQUAL (symbol_ref:SI ("dbs") [flags 0x40]  0x7ffb097e6ab0 dbs>)
> (nil)))
>
> and everything works fine.
>
> Bootstrapped and tested on aarch64-none-linux-gnu.
> Also tested on aarch64_be-none-elf.
> Ok for trunk?

Naveen posted the exact same patch:
https://gcc.gnu.org/ml/gcc-patches/2016-11/msg02305.html
Though with a slightly different testcase :).

Thanks,
Andrew

>
> Thanks,
> Kyrill
>
> 2016-11-29  Kyrylo Tkachov  
>
> PR target/71112
> * config/aarch64/aarch64.c (aarch64_load_symref_appropriately,
> case SYMBOL_SMALL_GOT_28K): Use gen_lowpart rather than constructing
> subreg directly.
>
> 2016-11-29  Kyrylo Tkachov  
>
> PR target/71112
> * gcc.c-torture/compile/pr71112.c: New test.