Message ID | 563A1597.9060604@foss.arm.com |
---|---|
State | New |
Headers | show |
On Wed, Nov 04, 2015 at 02:26:31PM +0000, Ramana Radhakrishnan wrote: > > > On 03/11/15 17:09, James Greenhalgh wrote: > > On Tue, Nov 03, 2015 at 04:36:45PM +0000, Ramana Radhakrishnan wrote: > >> Hi, > >> > >> Now that PR63304 is fixed and we have an option to address > >> any part of the memory using adrp / add or adrp / ldr instructions > >> it makes sense to switch out literal pools into their own > >> mergeable sections by default. > >> > >> This would mean that by default we could now start getting > >> the benefits of constant sharing across the board, potentially > >> improving code size. The other advantage of doing so, for the > >> security conscious is that this prevents intermingling of literal > >> pools with code. > >> > >> Wilco's kindly done some performance measurements and suggests that > >> there is not really a performance regression in doing this. > >> I've looked at the code size for SPEC2k6 today at -Ofast and > >> in general there is a good code size improvement as expected > >> by sharing said constants. > >> > >> Tested on aarch64-none-elf with no regressions and bootstrapped > >> and regression tested in my tree for a number of days now. > >> > >> Ok to commit ? > > > > OK with the comment nits below fixed up. > > > >> * config/aarch64/aarch64.c (aarch64_select_rtx_section): Switch > >> to default section handling for non PC relative literal loads. > > > >> diff --git a/gcc/config/aarch64/aarch64.c b/gcc/config/aarch64/aarch64.c > >> index 5c8604f..9d709e5 100644 > >> --- a/gcc/config/aarch64/aarch64.c > >> +++ b/gcc/config/aarch64/aarch64.c > >> @@ -5244,13 +5244,22 @@ aarch64_use_blocks_for_constant_p (machine_mode mode ATTRIBUTE_UNUSED, > >> return false; > >> } > >> > >> +/* Force all constant pool entries into the current function section. > > > > Is this comment accurate now? I think it only applied to -mcmodel=large > > but maybe I'm misunderstanding? > > > >> + In the large model we cannot reliably address all the address space > >> + thus for now, inline this with the text. */ > >> static section * > >> +aarch64_select_rtx_section (machine_mode mode, > >> + rtx x, > >> + unsigned HOST_WIDE_INT align) > >> +{ > >> + /* Force all constant pool entries into the current function section. > >> + In the large model we cannot reliably address all the address space > >> + thus for now, inline this with the text. */ > >> + if (!aarch64_nopcrelative_literal_loads > >> + || aarch64_cmodel == AARCH64_CMODEL_LARGE) > >> + return function_section (current_function_decl); > > > > This is just a copy paste of the text above (and probably the better place > > for it). > > > > I think we'd want a more general comment at the top of the function, > > then this can stay. > > > > True and I've just been reading more of the backend - We could now start > using blocks for constant pools as well. So let's do that. > > How does something like this look ? One tiny nit. Otherwise, this is OK. > Tested on aarch64-none-elf - no regressions. > > 2015-11-04 Ramana Radhakrishnan <ramana.radhakrishnan@arm.com> > > * config/aarch64/aarch64.c > (aarch64_can_use_per_function_literal_pools_p): New. > (aarch64_use_blocks_for_constant_p): Adjust declaration > and use aarch64_can_use_function_literal_pools_p. > (aarch64_select_rtx_section): Update. > diff --git a/gcc/config/aarch64/aarch64.c b/gcc/config/aarch64/aarch64.c > index 5c8604f..4f5e069 100644 > --- a/gcc/config/aarch64/aarch64.c > +++ b/gcc/config/aarch64/aarch64.c > @@ -5235,24 +5235,37 @@ aarch64_uxt_size (int shift, HOST_WIDE_INT mask) > return 0; > } > > +/* Constant pools are per function only when PC relative > + literal loads are true or we are in the large memory > + model. */ Newline here please. Thanks, James > +static inline bool > +aarch64_can_use_per_function_literal_pools_p (void) > +{ > + return (!aarch64_nopcrelative_literal_loads > + || aarch64_cmodel == AARCH64_CMODEL_LARGE); > +} > +
This is causing a bootstrap comparison failure in gcc/go/gogo.o. Andreas.
On 08/11/15 11:42, Andreas Schwab wrote: > This is causing a bootstrap comparison failure in gcc/go/gogo.o. I'm looking into this - this is now PR68256. regards Ramana > > Andreas. >
On 04/11/15 14:26, Ramana Radhakrishnan wrote: > > True and I've just been reading more of the backend - We could now start using blocks for constant pools as well. So let's do that. > > How does something like this look ? > > Tested on aarch64-none-elf - no regressions. > > 2015-11-04 Ramana Radhakrishnan <ramana.radhakrishnan@arm.com> > > * config/aarch64/aarch64.c > (aarch64_can_use_per_function_literal_pools_p): New. > (aarch64_use_blocks_for_constant_p): Adjust declaration > and use aarch64_can_use_function_literal_pools_p. > (aarch64_select_rtx_section): Update. > Since r229878, I've been seeing FAIL: gcc.dg/attr-weakref-1.c (test for excess errors) UNRESOLVED: gcc.dg/attr-weakref-1.c compilation failed to produce executable (both previously passing) on aarch64-none-elf, aarch64_be-none-elf, and aarch64-none-linux-gnu. Here's a log from aarch64_be-none-elf (the others look similar): /work/alalaw01/build-aarch64_be-none-elf/obj/gcc2/gcc/xgcc -B/work/alalaw01/build-aarch64_be-none-elf/obj/gcc2/gcc/ /work/alalaw01/src/gcc/gcc/testsuite/gcc.dg/attr-weakref-1.c -fno-diagnostics-show-caret -fdiagnostics-color=never -O2 /work/alalaw01/src/gcc/gcc/testsuite/gcc.dg/attr-weakref-1a.c -specs=aem-validation.specs -lm -o ./attr-weakref-1.exe /tmp/ccEfngi6.o:(.rodata.cst8+0x30): undefined reference to `wv12' /tmp/ccEfngi6.o:(.rodata.cst8+0x38): undefined reference to `wv12' /tmp/ccEfngi6.o:(.rodata.cst8+0x60): undefined reference to `wf12' /tmp/ccEfngi6.o:(.rodata.cst8+0x68): undefined reference to `wf12' collect2: error: ld returned 1 exit status compiler exited with status 1 output is: /tmp/ccEfngi6.o:(.rodata.cst8+0x30): undefined reference to `wv12' /tmp/ccEfngi6.o:(.rodata.cst8+0x38): undefined reference to `wv12' /tmp/ccEfngi6.o:(.rodata.cst8+0x60): undefined reference to `wf12' /tmp/ccEfngi6.o:(.rodata.cst8+0x68): undefined reference to `wf12' collect2: error: ld returned 1 exit status FAIL: gcc.dg/attr-weakref-1.c (test for excess errors)
On Tue, Nov 10, 2015 at 4:39 PM, Alan Lawrence <alan.lawrence@arm.com> wrote: > On 04/11/15 14:26, Ramana Radhakrishnan wrote: >> >> >> True and I've just been reading more of the backend - We could now start >> using blocks for constant pools as well. So let's do that. >> >> How does something like this look ? >> >> Tested on aarch64-none-elf - no regressions. >> >> 2015-11-04 Ramana Radhakrishnan <ramana.radhakrishnan@arm.com> >> >> * config/aarch64/aarch64.c >> (aarch64_can_use_per_function_literal_pools_p): New. >> (aarch64_use_blocks_for_constant_p): Adjust declaration >> and use aarch64_can_use_function_literal_pools_p. >> (aarch64_select_rtx_section): Update. >> > > Since r229878, I've been seeing > > FAIL: gcc.dg/attr-weakref-1.c (test for excess errors) > UNRESOLVED: gcc.dg/attr-weakref-1.c compilation failed to produce executable > > (both previously passing) on aarch64-none-elf, aarch64_be-none-elf, and > aarch64-none-linux-gnu. Here's a log from aarch64_be-none-elf (the others > look similar): > > /work/alalaw01/build-aarch64_be-none-elf/obj/gcc2/gcc/xgcc > -B/work/alalaw01/build-aarch64_be-none-elf/obj/gcc2/gcc/ > /work/alalaw01/src/gcc/gcc/testsuite/gcc.dg/attr-weakref-1.c > -fno-diagnostics-show-caret -fdiagnostics-color=never -O2 > /work/alalaw01/src/gcc/gcc/testsuite/gcc.dg/attr-weakref-1a.c > -specs=aem-validation.specs -lm -o ./attr-weakref-1.exe > /tmp/ccEfngi6.o:(.rodata.cst8+0x30): undefined reference to `wv12' > /tmp/ccEfngi6.o:(.rodata.cst8+0x38): undefined reference to `wv12' > /tmp/ccEfngi6.o:(.rodata.cst8+0x60): undefined reference to `wf12' > /tmp/ccEfngi6.o:(.rodata.cst8+0x68): undefined reference to `wf12' > collect2: error: ld returned 1 exit status > compiler exited with status 1 > output is: > /tmp/ccEfngi6.o:(.rodata.cst8+0x30): undefined reference to `wv12' > /tmp/ccEfngi6.o:(.rodata.cst8+0x38): undefined reference to `wv12' > /tmp/ccEfngi6.o:(.rodata.cst8+0x60): undefined reference to `wf12' > /tmp/ccEfngi6.o:(.rodata.cst8+0x68): undefined reference to `wf12' > collect2: error: ld returned 1 exit status > > FAIL: gcc.dg/attr-weakref-1.c (test for excess errors) > Hmmm I'm surprised it failed in the first place as my testing didn't show it - I need to check on that. Nevertheless this fail has gone away in my testing with https://gcc.gnu.org/ml/gcc-cvs/2015-11/msg00453.html in a bootstrap and regression run on aarch64-none-linux-gnu. I see nothing triplet specific in the testcase here for it to fail differently. Is this something you see really with tip of trunk ? regards Ramana
On 10/11/15 16:39, Alan Lawrence wrote: > Since r229878, I've been seeing > > FAIL: gcc.dg/attr-weakref-1.c (test for excess errors) > UNRESOLVED: gcc.dg/attr-weakref-1.c compilation failed to produce executable > > (both previously passing) on aarch64-none-elf, aarch64_be-none-elf, and > aarch64-none-linux-gnu. Ah, these are fixed by Ramana's partial rollback (r230085). --Alan
diff --git a/gcc/config/aarch64/aarch64.c b/gcc/config/aarch64/aarch64.c index 5c8604f..4f5e069 100644 --- a/gcc/config/aarch64/aarch64.c +++ b/gcc/config/aarch64/aarch64.c @@ -5235,24 +5235,37 @@ aarch64_uxt_size (int shift, HOST_WIDE_INT mask) return 0; } +/* Constant pools are per function only when PC relative + literal loads are true or we are in the large memory + model. */ +static inline bool +aarch64_can_use_per_function_literal_pools_p (void) +{ + return (!aarch64_nopcrelative_literal_loads + || aarch64_cmodel == AARCH64_CMODEL_LARGE); +} + static bool -aarch64_use_blocks_for_constant_p (machine_mode mode ATTRIBUTE_UNUSED, - const_rtx x ATTRIBUTE_UNUSED) +aarch64_use_blocks_for_constant_p (machine_mode, const_rtx) { /* We can't use blocks for constants when we're using a per-function constant pool. */ - return false; + return !aarch64_can_use_per_function_literal_pools_p (); } +/* Select appropriate section for constants depending + on where we place literal pools. */ + static section * -aarch64_select_rtx_section (machine_mode mode ATTRIBUTE_UNUSED, - rtx x ATTRIBUTE_UNUSED, - unsigned HOST_WIDE_INT align ATTRIBUTE_UNUSED) +aarch64_select_rtx_section (machine_mode mode, + rtx x, + unsigned HOST_WIDE_INT align) { - /* Force all constant pool entries into the current function section. */ - return function_section (current_function_decl); -} + if (aarch64_can_use_per_function_literal_pools_p ()) + return function_section (current_function_decl); + return default_elf_select_rtx_section (mode, x, align); +} /* Costs. */