| Message ID | CO2PR07MB269488C6223A95A66CCCBC2B83B70@CO2PR07MB2694.namprd07.prod.outlook.com |
|---|---|
| State | Superseded |
| Headers | show |
[CC'ing James] On 23/11/16 05:25, Hurugalawadi, Naveen wrote: > Hi, > > Please find attached the patch that fixes PR71112. > > The current implementation that handles SYMBOL_SMALL_GOT_28K in > aarch64_load_symref_appropriately access the high part of RTX for Big-Endian > mode which results in ICE for ILP32. > > The attached patch modifies it by accessing the lower part for both Endian > and fixes the issue. > > Please review the patch and let me know if its okay? This looks ok to me as I had independently come up with an identical patch for it. But I can't approve. Thanks, Kyrill > > 2016-11-23 Andrew PInski <apinski@cavium.com> > > gcc > * config/aarch64/aarch64.c (aarch64_load_symref_appropriately): > Access the lower part of RTX appropriately. > > gcc/testsuite > * gcc.target/aarch64/pr71112.c : New Testcase.
On Wed, Nov 23, 2016 at 05:25:44AM +0000, Hurugalawadi, Naveen wrote: > Hi, > > Please find attached the patch that fixes PR71112. > > The current implementation that handles SYMBOL_SMALL_GOT_28K in > aarch64_load_symref_appropriately access the high part of RTX for Big-Endian > mode which results in ICE for ILP32. > > The attached patch modifies it by accessing the lower part for both Endian > and fixes the issue. > > Please review the patch and let me know if its okay? > > > 2016-11-23 Andrew PInski <apinski@cavium.com> > > gcc > * config/aarch64/aarch64.c (aarch64_load_symref_appropriately): > Access the lower part of RTX appropriately. > > gcc/testsuite > * gcc.target/aarch64/pr71112.c : New Testcase. > diff --git a/gcc/config/aarch64/aarch64.c b/gcc/config/aarch64/aarch64.c > index efcba83..4d87953 100644 > --- a/gcc/config/aarch64/aarch64.c > +++ b/gcc/config/aarch64/aarch64.c > @@ -1298,7 +1298,8 @@ aarch64_load_symref_appropriately (rtx dest, rtx imm, > emit_move_insn (gp_rtx, gen_rtx_HIGH (Pmode, s)); > > if (mode != GET_MODE (gp_rtx)) > - gp_rtx = simplify_gen_subreg (mode, gp_rtx, GET_MODE (gp_rtx), 0); > + gp_rtx = gen_lowpart (mode, gp_rtx); > + > } > > if (mode == ptr_mode) > diff --git a/gcc/testsuite/gcc.target/aarch64/pr71112.c b/gcc/testsuite/gcc.target/aarch64/pr71112.c > new file mode 100644 > index 0000000..5bb9dee > --- /dev/null > +++ b/gcc/testsuite/gcc.target/aarch64/pr71112.c > @@ -0,0 +1,12 @@ > +/* PR target/71112 */ > +/* { dg-do compile } */ > +/* { dg-options "-mabi=ilp32 -mbig-endian -fpie" } */ Why limit the ABI and endianness here, and if you do plan to do that should you not also check the effective target to make sure these options are OK to add? i.e. /* { dg-require-effective-target pie } */ /* { dg-require-effective-target aarch64_big_endian } */ And probably one for ilp32 too? As this testcase should pass across all ABI variants and all endinaness the right thing to do is probably to drop the extra options. The same comment applies to the testcase for PR78382. I'm concerned we get this right early, as the trouble caused in the ARM back-end by testcases assuming they can add options, then FAILing (for a variety of reasons) is very painful, and makes it hard to read test output on ARM - I'd rather we didn't introduce the same issues on AArch64. Thanks, James
On Wed, Nov 23, 2016 at 5:25 AM, Hurugalawadi, Naveen <Naveen.Hurugalawadi@cavium.com> wrote: > Hi, > > Please find attached the patch that fixes PR71112. > > The current implementation that handles SYMBOL_SMALL_GOT_28K in > aarch64_load_symref_appropriately access the high part of RTX for Big-Endian > mode which results in ICE for ILP32. > > The attached patch modifies it by accessing the lower part for both Endian > and fixes the issue. > > Please review the patch and let me know if its okay? > PR71112 is still open - should this be backported to GCC-6 ? regards Ramana > > 2016-11-23 Andrew PInski <apinski@cavium.com> > > gcc > * config/aarch64/aarch64.c (aarch64_load_symref_appropriately): > Access the lower part of RTX appropriately. > > gcc/testsuite > * gcc.target/aarch64/pr71112.c : New Testcase.
Hi Ramana,
>> PR71112 is still open - should this be backported to GCC-6 ?
Ported the patch to gcc-6-branch and committed as:-
https://gcc.gnu.org/viewcvs/gcc?view=revision&revision=250014
Bootstrapped and Regression Tested gcc-6-branch for AArch64
on aarch64-thunder-linux.
Thanks,
Naveen
diff --git a/gcc/config/aarch64/aarch64.c b/gcc/config/aarch64/aarch64.c index efcba83..4d87953 100644 --- a/gcc/config/aarch64/aarch64.c +++ b/gcc/config/aarch64/aarch64.c @@ -1298,7 +1298,8 @@ aarch64_load_symref_appropriately (rtx dest, rtx imm, emit_move_insn (gp_rtx, gen_rtx_HIGH (Pmode, s)); if (mode != GET_MODE (gp_rtx)) - gp_rtx = simplify_gen_subreg (mode, gp_rtx, GET_MODE (gp_rtx), 0); + gp_rtx = gen_lowpart (mode, gp_rtx); + } if (mode == ptr_mode) diff --git a/gcc/testsuite/gcc.target/aarch64/pr71112.c b/gcc/testsuite/gcc.target/aarch64/pr71112.c new file mode 100644 index 0000000..5bb9dee --- /dev/null +++ b/gcc/testsuite/gcc.target/aarch64/pr71112.c @@ -0,0 +1,12 @@ +/* PR target/71112 */ +/* { dg-do compile } */ +/* { dg-options "-mabi=ilp32 -mbig-endian -fpie" } */ + +extern int dbs[100]; +void f (int *); +int +nscd_init (void) +{ + f (dbs); + return 0; +}