| Message ID | 58765E2D.5030609@foss.arm.com |
|---|---|
| State | New |
| Headers | show |
On Wed, Jan 11, 2017 at 04:32:45PM +0000, Kyrill Tkachov wrote: > Hi all, > > In this PR we generated ADRP/ADD instructions with :lo12: relocations on > symbols even though -mpc-relative-literal-loads is used. This is due to the > confusing double-negative logic of the nopcrelative_literal_loads aarch64 > variable and its relation to the aarch64_nopcrelative_literal_loads global > variable in the GCC 6 branch. > > Wilco fixed this on trunk as part of a larger patch (r237607) and parts of > that patch were backported, but other parts weren't and that patch now > doesn't apply cleanly to the branch. As I commented to Jakub at the time he made the first partial backport, I'd much rather just see all of Wilco's patch backported. We're not on the verge of a 6 release now, so please just backport Wilco's patch (as should have been done all along if this had been correctly identified as a fix rather than a clean-up). Thanks, James
On 13/01/17 16:35, James Greenhalgh wrote: > On Wed, Jan 11, 2017 at 04:32:45PM +0000, Kyrill Tkachov wrote: >> Hi all, >> >> In this PR we generated ADRP/ADD instructions with :lo12: relocations on >> symbols even though -mpc-relative-literal-loads is used. This is due to the >> confusing double-negative logic of the nopcrelative_literal_loads aarch64 >> variable and its relation to the aarch64_nopcrelative_literal_loads global >> variable in the GCC 6 branch. >> >> Wilco fixed this on trunk as part of a larger patch (r237607) and parts of >> that patch were backported, but other parts weren't and that patch now >> doesn't apply cleanly to the branch. > As I commented to Jakub at the time he made the first partial backport, > I'd much rather just see all of Wilco's patch backported. We're not on > the verge of a 6 release now, so please just backport Wilco's patch (as > should have been done all along if this had been correctly identified as > a fix rather than a clean-up). Unfortunately simply backporting the patch does not fix this PR. The aarch64_classify_symbol changes do not have the desired effect and the :lo12: relocations are generated. I'll look into it, but I believe that would require a bigger change than this one-liner. Thanks, Kyri > Thanks, > James > >
diff --git a/gcc/config/aarch64/aarch64.c b/gcc/config/aarch64/aarch64.c index 83dbd57..fa61289 100644 --- a/gcc/config/aarch64/aarch64.c +++ b/gcc/config/aarch64/aarch64.c @@ -9324,7 +9324,7 @@ aarch64_classify_symbol (rtx x, rtx offset) /* This is alright even in PIC code as the constant pool reference is always PC relative and within the same translation unit. */ - if (nopcrelative_literal_loads + if (aarch64_nopcrelative_literal_loads && CONSTANT_POOL_ADDRESS_P (x)) return SYMBOL_SMALL_ABSOLUTE; else diff --git a/gcc/testsuite/gcc.target/aarch64/pr79041.c b/gcc/testsuite/gcc.target/aarch64/pr79041.c new file mode 100644 index 0000000..a23b1ae --- /dev/null +++ b/gcc/testsuite/gcc.target/aarch64/pr79041.c @@ -0,0 +1,26 @@ +/* PR target/79041. Check that we don't generate the LO12 relocations + for -mpc-relative-literal-loads. */ +/* { dg-do compile } */ +/* { dg-options "-O2 -mcmodel=large -mpc-relative-literal-loads" } */ + +extern int strcmp (const char *, const char *); +extern char *strcpy (char *, const char *); + +static struct +{ + char *b; + char *c; +} d[] = { + {"0", "000000000000000"}, {"1", "111111111111111"}, +}; + +void +e (const char *b, char *c) +{ + int i; + for (i = 0; i < 1; ++i) + if (!strcmp (d[i].b, b)) + strcpy (c, d[i].c); +} + +/* { dg-final { scan-assembler-not ":lo12:" } } */
Hi all, In this PR we generated ADRP/ADD instructions with :lo12: relocations on symbols even though -mpc-relative-literal-loads is used. This is due to the confusing double-negative logic of the nopcrelative_literal_loads aarch64 variable and its relation to the aarch64_nopcrelative_literal_loads global variable in the GCC 6 branch. Wilco fixed this on trunk as part of a larger patch (r237607) and parts of that patch were backported, but other parts weren't and that patch now doesn't apply cleanly to the branch. The actual bug here is that aarch64_classify_symbol uses nopcrelative_literal_loads instead of the correct aarch64_nopcrelative_literal_loads. nopcrelative_literal_loads gets set to 1 if the user specifies -mpc-relative-literal-loads(!) whereas aarch64_nopcrelative_literal_loads gets set to false, so that is the variable we want to check. So this is the minimal patch that fixes this. Bootstrapped and tested on aarch64-none-linux-gnu on the GCC 6 branch. Ok for the branch? Thanks, Kyrill 2017-01-11 Kyrylo Tkachov <kyrylo.tkachov@arm.com> PR target/79041 * config/aarch64/aarch64.c (aarch64_classify_symbol): Use aarch64_nopcrelative_literal_loads instead of nopcrelative_literal_loads. 2017-01-11 Kyrylo Tkachov <kyrylo.tkachov@arm.com> PR target/79041 * gcc.target/aarch64/pr79041.c: New test.