Re: [PATCH] [AArch64] Fix PR71112
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
Re: [PATCH] [AArch64] Fix PR71112
On Wed, Nov 23, 2016 at 5:25 AM, 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? > PR71112 is still open - should this be backported to GCC-6 ? regards Ramana > > 2016-11-23 Andrew PInski > > 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.
Re: [PATCH] [AArch64] Fix PR71112
On Wed, Dec 07, 2016 at 07:25:21AM +, Hurugalawadi, Naveen wrote:
> Hi James,
>
> Thanks for the review and suggestions regarding the testcase.
>
> >> Why limit the ABI and endianness here, and if you do plan to do that
>
> Extra options have been dropped and the testcase will check across
> all variants and endianness.
>
> Please find attached the modified patch as per the comments and let
> me know if its okay?
OK with an appropriate ChangeLog entry.
Thanks,
James
> diff --git a/gcc/config/aarch64/aarch64.c b/gcc/config/aarch64/aarch64.c
> index dab46b5..9fce849 100644
> --- a/gcc/config/aarch64/aarch64.c
> +++ b/gcc/config/aarch64/aarch64.c
> @@ -1302,7 +1302,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.c-torture/compile/pr71112.c
> b/gcc/testsuite/gcc.c-torture/compile/pr71112.c
> new file mode 100644
> index 000..69e2df6
> --- /dev/null
> +++ b/gcc/testsuite/gcc.c-torture/compile/pr71112.c
> @@ -0,0 +1,10 @@
> +/* PR target/71112. */
> +/* { dg-additional-options "-fpie" { target pie } } */
> +
> +extern int dbs[100];
> +void f (int *);
> +int nscd_init (void)
> +{
> + f (dbs);
> + return 0;
> +}
Re: [PATCH] [AArch64] Fix PR71112
Hi James,
Thanks for the review and suggestions regarding the testcase.
>> Why limit the ABI and endianness here, and if you do plan to do that
Extra options have been dropped and the testcase will check across
all variants and endianness.
Please find attached the modified patch as per the comments and let
me know if its okay?
Thanks,
Naveen
diff --git a/gcc/config/aarch64/aarch64.c b/gcc/config/aarch64/aarch64.c
index dab46b5..9fce849 100644
--- a/gcc/config/aarch64/aarch64.c
+++ b/gcc/config/aarch64/aarch64.c
@@ -1302,7 +1302,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.c-torture/compile/pr71112.c b/gcc/testsuite/gcc.c-torture/compile/pr71112.c
new file mode 100644
index 000..69e2df6
--- /dev/null
+++ b/gcc/testsuite/gcc.c-torture/compile/pr71112.c
@@ -0,0 +1,10 @@
+/* PR target/71112. */
+/* { dg-additional-options "-fpie" { target pie } } */
+
+extern int dbs[100];
+void f (int *);
+int nscd_init (void)
+{
+ f (dbs);
+ return 0;
+}
Re: [PATCH] [AArch64] Fix PR71112
On Wed, Nov 23, 2016 at 05:25:44AM +, 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
>
> 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 000..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
Re: [PATCH] [AArch64] Fix PR71112
[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 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.
