[Patch 1/2 PR78561] Rename get_pool_size to get_pool_size_upper_bound

[Patch 1/2 PR78561] Rename get_pool_size to get_pool_size_upper_bound

[James Greenhalgh]

[-- Attachment #1: Type: text/plain, Size: 1221 bytes --]


Hi,

There's no functional change in this patch, just a rename.

The size recorded in "offset" is only ever incremented as we add new items
to the constant pool. But this information can become stale where those
constant pool entries would not get emitted.

Thus, it is only ever an upper bound on the size of the constant pool.

The only uses of get_pool_size are in rs6000 and there it is only used to
check whether a constant pool might be output - but explicitly renaming the
function to make it clear that you're getting an upper bound rather than the
real size can only be good for programmers using the interface.

OK?

Thanks,
James

---
2016-12-01  James Greenhalgh  <james.greenhalgh@arm.com>

	PR rtl-optimization/78561
	* config/rs6000/rs6000.c (rs6000_reg_live_or_pic_offset_p) Rename
	get_pool_size to get_pool_size_upper_bound.
	(rs6000_stack_info): Likewise.
	(rs6000_emit_prologue): Likewise.
	(rs6000_elf_declare_function_name): Likewise.
	(rs6000_set_up_by_prologue): Likewise.
	(rs6000_can_eliminate): Likewise, reformat spaces to tabs.
	* output.h (get_pool_size): Rename to...
	(get_pool_size_upper_bound): ...This.
	* varasm.c (get_pool_size): Rename to...
	(get_pool_size_upper_bound): ...This.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Patch-1-2-PR78561-Rename-get_pool_size-to-get_pool_s.patch --]
[-- Type: text/x-patch; name="0001-Patch-1-2-PR78561-Rename-get_pool_size-to-get_pool_s.patch", Size: 4147 bytes --]

diff --git a/gcc/config/rs6000/rs6000.c b/gcc/config/rs6000/rs6000.c
index 0a6a784..7e965f9 100644
--- a/gcc/config/rs6000/rs6000.c
+++ b/gcc/config/rs6000/rs6000.c
@@ -25456,7 +25456,7 @@ rs6000_reg_live_or_pic_offset_p (int reg)
       if (TARGET_TOC && TARGET_MINIMAL_TOC
 	  && (crtl->calls_eh_return
 	      || df_regs_ever_live_p (reg)
-	      || get_pool_size ()))
+	      || get_pool_size_upper_bound ()))
 	return true;
 
       if ((DEFAULT_ABI == ABI_V4 || DEFAULT_ABI == ABI_DARWIN)
@@ -26262,7 +26262,7 @@ rs6000_stack_info (void)
 #ifdef TARGET_RELOCATABLE
       || (DEFAULT_ABI == ABI_V4
 	  && (TARGET_RELOCATABLE || flag_pic > 1)
-	  && get_pool_size () != 0)
+	  && get_pool_size_upper_bound () != 0)
 #endif
       || rs6000_ra_ever_killed ())
     info->lr_save_p = 1;
@@ -28039,7 +28039,8 @@ rs6000_emit_prologue (void)
       cfun->machine->r2_setup_needed = df_regs_ever_live_p (TOC_REGNUM);
 
       /* With -mminimal-toc we may generate an extra use of r2 below.  */
-      if (TARGET_TOC && TARGET_MINIMAL_TOC && get_pool_size () != 0)
+      if (TARGET_TOC && TARGET_MINIMAL_TOC
+	  && get_pool_size_upper_bound () != 0)
 	cfun->machine->r2_setup_needed = true;
     }
 
@@ -28894,7 +28895,8 @@ rs6000_emit_prologue (void)
 
   /* If we are using RS6000_PIC_OFFSET_TABLE_REGNUM, we need to set it up.  */
   if (!TARGET_SINGLE_PIC_BASE
-      && ((TARGET_TOC && TARGET_MINIMAL_TOC && get_pool_size () != 0)
+      && ((TARGET_TOC && TARGET_MINIMAL_TOC
+	   && get_pool_size_upper_bound () != 0)
 	  || (DEFAULT_ABI == ABI_V4
 	      && (flag_pic == 1 || (flag_pic && TARGET_SECURE_PLT))
 	      && df_regs_ever_live_p (RS6000_PIC_OFFSET_TABLE_REGNUM))))
@@ -34961,7 +34963,7 @@ rs6000_elf_declare_function_name (FILE *file, const char *name, tree decl)
   if (DEFAULT_ABI == ABI_V4
       && (TARGET_RELOCATABLE || flag_pic > 1)
       && !TARGET_SECURE_PLT
-      && (get_pool_size () != 0 || crtl->profile)
+      && (get_pool_size_upper_bound () != 0 || crtl->profile)
       && uses_TOC ())
     {
       char buf[256];
@@ -37444,10 +37446,11 @@ static bool
 rs6000_can_eliminate (const int from, const int to)
 {
   return (from == ARG_POINTER_REGNUM && to == STACK_POINTER_REGNUM
-          ? ! frame_pointer_needed
-          : from == RS6000_PIC_OFFSET_TABLE_REGNUM
-            ? ! TARGET_MINIMAL_TOC || TARGET_NO_TOC || get_pool_size () == 0
-            : true);
+	  ? ! frame_pointer_needed
+	  : from == RS6000_PIC_OFFSET_TABLE_REGNUM
+	    ? ! TARGET_MINIMAL_TOC || TARGET_NO_TOC
+		|| get_pool_size_upper_bound () == 0
+	    : true);
 }
 
 /* Define the offset between two registers, FROM to be eliminated and its
@@ -38983,7 +38986,7 @@ rs6000_set_up_by_prologue (struct hard_reg_set_container *set)
   if (!TARGET_SINGLE_PIC_BASE
       && TARGET_TOC
       && TARGET_MINIMAL_TOC
-      && get_pool_size () != 0)
+      && get_pool_size_upper_bound () != 0)
     add_to_hard_reg_set (&set->set, Pmode, RS6000_PIC_OFFSET_TABLE_REGNUM);
   if (cfun->machine->split_stack_argp_used)
     add_to_hard_reg_set (&set->set, Pmode, 12);
diff --git a/gcc/output.h b/gcc/output.h
index 0924499..7186dc1 100644
--- a/gcc/output.h
+++ b/gcc/output.h
@@ -287,8 +287,11 @@ extern void assemble_real (REAL_VALUE_TYPE, machine_mode, unsigned,
 /* Write the address of the entity given by SYMBOL to SEC.  */
 extern void assemble_addr_to_section (rtx, section *);
 
-/* Return the size of the constant pool.  */
-extern int get_pool_size (void);
+/* Return the maximum size of the constant pool.  This may be larger
+   than the final size of the constant pool, as entries may be added to
+   the constant pool which become unreferenced, or otherwise not need
+   output by the time we actually emit the pool.  */
+extern int get_pool_size_upper_bound (void);
 
 extern rtx_insn *peephole (rtx_insn *);
 
diff --git a/gcc/varasm.c b/gcc/varasm.c
index 1e7c2b5..f8af0c1 100644
--- a/gcc/varasm.c
+++ b/gcc/varasm.c
@@ -3811,7 +3811,7 @@ get_pool_mode (const_rtx addr)
 /* Return the size of the constant pool.  */
 
 int
-get_pool_size (void)
+get_pool_size_upper_bound (void)
 {
   return crtl->varasm.pool->offset;
 }

[Patch 0/2 PR78561] Recalculate constant pool size before emitting it

[James Greenhalgh]

[-- Attachment #1: Type: text/plain, Size: 2496 bytes --]

Hi,

In PR78561, we try to make use of stale constant pool offset data when
making decisions as to whether to output an alignment directive after
the AArch64 constant pool. The offset data goes stale as we only ever
increment it when adding new constants to the pool (it represents an
upper bound on the size of the pool).

To fix that, we should recompute the offset values shortly after
sweeping through insns looking for valid constant.

That's easy enough to do (see patch 2/2) and patch 1/2 is just a simple
rename of the get_pool_size function to reflect that it is not providing
an accurate size, just an upper bound on what the size might be after
optimisation.

Technically, patch 1/2 isn't neccessary to fix the PR but cleaning up the
name seems like a useful to do.

The patch set has been bootstrapped and tested on aarch64-none-linux-gnu and
x86-64-none-linux-gnu without any issues. I've also cross-tested it for
aarch64-none-elf and build-tested it for rs6000 (though I couldn't run the
testsuite as I don't have a test environment).

OK?

Thanks,
James

---

[Patch 1/2 PR78561] Rename get_pool_size to get_pool_size_upper_bound

gcc/

2016-12-01  James Greenhalgh  <james.greenhalgh@arm.com>

	PR rtl-optimization/78561
	* config/rs6000/rs6000.c (rs6000_reg_live_or_pic_offset_p) Rename
	get_pool_size to get_pool_size_upper_bound.
	(rs6000_stack_info): Likewise.
	(rs6000_emit_prologue): Likewise.
	(rs6000_elf_declare_function_name): Likewise.
	(rs6000_set_up_by_prologue): Likewise.
	(rs6000_can_eliminate): Likewise, reformat spaces to tabs.
	* output.h (get_pool_size): Rename to...
	(get_pool_size_upper_bound): ...This.
	* varasm.c (get_pool_size): Rename to...
	(get_pool_size_upper_bound): ...This.

[Patch 2/2 PR78561] Recalculate constant pool size before emitting it

gcc/

2016-12-01  James Greenhalgh  <james.greenhalgh@arm.com>

	PR rtl-optimization/78561
	* varasm.c (recompute_pool_offsets): New.
	(output_constant_pool): Call it.

gcc/testsuite/

2016-12-01  James Greenhalgh  <james.greenhalgh@arm.com>

	PR rtl-optimization/78561
	* gcc.target/aarch64/pr78561.c: New.

---

 gcc/config/rs6000/rs6000.c                 | 23 +++++++++++++----------
 gcc/output.h                               |  7 +++++--
 gcc/testsuite/gcc.target/aarch64/pr78561.c |  9 +++++++++
 gcc/varasm.c                               | 30 +++++++++++++++++++++++++++++-
 4 files changed, 56 insertions(+), 13 deletions(-)
 create mode 100644 gcc/testsuite/gcc.target/aarch64/pr78561.c

[Patch 2/2 PR78561] Recalculate constant pool size before emitting it

[James Greenhalgh]

[-- Attachment #1: Type: text/plain, Size: 988 bytes --]


Hi,

In PR78561, we try to make use of stale constant pool offset data when
making decisions as to whether to output an alignment directive after
the AArch64 constant pool. The offset data goes stale as we only ever
increment it when adding new constants to the pool (it represents an
upper bound on the size of the pool).

To fix that, we should recompute the offset values shortly after
sweeping through insns looking for valid constant.

I'm not totally sure about this code so I'd appreciate comments on whether
this is a sensible idea.

Bootstrapped on aarch64-none-linux-gnu and x86-64-none-linux-gnu and
checked with aarch64-none-elf with no issues.

OK?

Thanks,
James

gcc/

2016-12-01  James Greenhalgh  <james.greenhalgh@arm.com>

	PR rtl-optimization/78561
	* varasm.c (recompute_pool_offsets): New.
	(output_constant_pool): Call it.

gcc/testsuite/

2016-12-01  James Greenhalgh  <james.greenhalgh@arm.com>

	PR rtl-optimization/78561
	* gcc.target/aarch64/pr78561.c: New.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0002-Patch-2-2-PR78561-Recalculate-constant-pool-size-bef.patch --]
[-- Type: text/x-patch; name="0002-Patch-2-2-PR78561-Recalculate-constant-pool-size-bef.patch", Size: 1866 bytes --]

diff --git a/gcc/testsuite/gcc.target/aarch64/pr78561.c b/gcc/testsuite/gcc.target/aarch64/pr78561.c
new file mode 100644
index 0000000..048d2d7
--- /dev/null
+++ b/gcc/testsuite/gcc.target/aarch64/pr78561.c
@@ -0,0 +1,9 @@
+/* { dg-do compile } */
+/* { dg-options "-Og -O3 -mcmodel=tiny" } */
+
+int
+main (__fp16 x)
+{
+  __fp16 a = 6.5504e4;
+  return (x <= a);
+}
diff --git a/gcc/varasm.c b/gcc/varasm.c
index f8af0c1..f3cd70a 100644
--- a/gcc/varasm.c
+++ b/gcc/varasm.c
@@ -3942,6 +3942,29 @@ output_constant_pool_1 (struct constant_descriptor_rtx *desc,
   return;
 }
 
+/* Recompute the offsets of entries in POOL, and the overall size of
+   POOL.  Do this after calling mark_constant_pool to ensure that we
+   are computing the offset values for the pool which we will actually
+   emit.  */
+
+static void
+recompute_pool_offsets (struct rtx_constant_pool *pool)
+{
+  struct constant_descriptor_rtx *desc;
+  pool->offset = 0;
+
+  for (desc = pool->first; desc ; desc = desc->next)
+    if (desc->mark)
+      {
+	  /* Recalculate offset.  */
+	unsigned int align = desc->align;
+	pool->offset += (align / BITS_PER_UNIT) - 1;
+	pool->offset &= ~ ((align / BITS_PER_UNIT) - 1);
+	desc->offset = pool->offset;
+	pool->offset += GET_MODE_SIZE (desc->mode);
+      }
+}
+
 /* Mark all constants that are referenced by SYMBOL_REFs in X.
    Emit referenced deferred strings.  */
 
@@ -4060,6 +4083,11 @@ output_constant_pool (const char *fnname ATTRIBUTE_UNUSED,
      case we do not need to output the constant.  */
   mark_constant_pool ();
 
+  /* Having marked the constant pool entries we'll actually emit, we
+     now need to rebuild the offset information, which may have become
+     stale.  */
+  recompute_pool_offsets (pool);
+
 #ifdef ASM_OUTPUT_POOL_PROLOGUE
   ASM_OUTPUT_POOL_PROLOGUE (asm_out_file, fnname, fndecl, pool->offset);
 #endif

Re: [Patch 1/2 PR78561] Rename get_pool_size to get_pool_size_upper_bound

[Jeff Law]

On 12/01/2016 08:29 AM, James Greenhalgh wrote:
>
> Hi,
>
> There's no functional change in this patch, just a rename.
>
> The size recorded in "offset" is only ever incremented as we add new items
> to the constant pool. But this information can become stale where those
> constant pool entries would not get emitted.
>
> Thus, it is only ever an upper bound on the size of the constant pool.
>
> The only uses of get_pool_size are in rs6000 and there it is only used to
> check whether a constant pool might be output - but explicitly renaming the
> function to make it clear that you're getting an upper bound rather than the
> real size can only be good for programmers using the interface.
>
> OK?
>
> Thanks,
> James
>
> ---
> 2016-12-01  James Greenhalgh  <james.greenhalgh@arm.com>
>
> 	PR rtl-optimization/78561
> 	* config/rs6000/rs6000.c (rs6000_reg_live_or_pic_offset_p) Rename
> 	get_pool_size to get_pool_size_upper_bound.
> 	(rs6000_stack_info): Likewise.
> 	(rs6000_emit_prologue): Likewise.
> 	(rs6000_elf_declare_function_name): Likewise.
> 	(rs6000_set_up_by_prologue): Likewise.
> 	(rs6000_can_eliminate): Likewise, reformat spaces to tabs.
> 	* output.h (get_pool_size): Rename to...
> 	(get_pool_size_upper_bound): ...This.
> 	* varasm.c (get_pool_size): Rename to...
> 	(get_pool_size_upper_bound): ...This.
>
Both parts of the fix for 78561 are OK.

Thanks,
Jeff

Re: [Patch 0/2 PR78561] Recalculate constant pool size before emitting it

[James Greenhalgh]

On Thu, Dec 01, 2016 at 12:55:21PM -0500, David Edelsohn wrote:
> >>>>> James Greenhalgh writes:
> 
> > The patch set has been bootstrapped and tested on aarch64-none-linux-gnu and
> > x86-64-none-linux-gnu without any issues. I've also cross-tested it for
> > aarch64-none-elf and build-tested it for rs6000 (though I couldn't run the
> > testsuite as I don't have a test environment).
> 
> There are PPC64 Linux and AIX systems in the GNU Compile Farm.  All
> have DejaGNU installed.

I bootstrapped and tested a compiler with these patches applied on
gcc112, there were no issues.

I've applied the patches based on Jeff's OK as revisions 243182, 243183.

Thanks,
James

Re: [Patch 0/2 PR78561] Recalculate constant pool size before emitting it

[David Edelsohn]

>>>>> James Greenhalgh writes:

> The patch set has been bootstrapped and tested on aarch64-none-linux-gnu and
> x86-64-none-linux-gnu without any issues. I've also cross-tested it for
> aarch64-none-elf and build-tested it for rs6000 (though I couldn't run the
> testsuite as I don't have a test environment).

There are PPC64 Linux and AIX systems in the GNU Compile Farm.  All
have DejaGNU installed.

- David