Re: [Patch AArch64] Switch constant pools to separate rodata sections.

[James Greenhalgh <james dot greenhalgh at arm dot com>]

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