diff mbox

[AArch64] Switch constant pools to separate rodata sections.

Message ID 5638E29D.6030704@foss.arm.com
State New
Headers show

Commit Message

Ramana Radhakrishnan Nov. 3, 2015, 4:36 p.m. UTC
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 ?

regards
Ramana

   * config/aarch64/aarch64.c (aarch64_select_rtx_section): Switch
to default section handling for non PC relative literal loads.

Comments

James Greenhalgh Nov. 3, 2015, 5:09 p.m. UTC | #1
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.

Thanks,
James
diff mbox

Patch

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.
+   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 ATTRIBUTE_UNUSED,
-			    rtx x ATTRIBUTE_UNUSED,
-			    unsigned HOST_WIDE_INT align ATTRIBUTE_UNUSED)
-{
-  /* Force all constant pool entries into the current function section.  */
-  return function_section (current_function_decl);
+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);
+
+  return default_elf_select_rtx_section (mode, x, align);
 }