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.radhakrish...@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);
> +}
> +
