diff mbox

[PR78561,PowerPC] Revert to old behaviour for counting constant pools

Message ID 1481106674-9787-1-git-send-email-james.greenhalgh@arm.com
State New
Headers show

Commit Message

James Greenhalgh Dec. 7, 2016, 10:31 a.m. UTC 
Hi,

This patch is a bit of a shot in the dark, as I've been unable to reproduce
Segher's failures reported in the PR. But, he has confirmed on the PR that
this patch fixes the issue he was seeing.

Before my patch set, rs6000 was reading the size of the constant pool at
several points in compilation, and expecting that if it read non-zero at one
point in compilation, it would always read non-zero.

That doesn't hold if we recompute the size of the constant pool just before
emitting it and decide that actually the size is zero. That can mean some
of the preparation statements emitted early in the PowerPC back end can think
a constant pool is needed, while later passes know there won't be one as all
the references are unused. That then causes issues for PIC register set up.

We always keep the entries in the constant pool after we first see them, and
we figure out what gets used in the constant pool by sweeping through
looking for referenced entries. That means we can always answer the
question "has the constant pool ever had entries?" by checking if there
is anything at all in there.

So that's what this patch does.

Bootstrapped on gcc110 (powerpc64-none-linux-gnu) configured with
--enable-languages=all,ada,go,obj-c++ with no issues, Segher has also
tested it with no issues, and I've tested an aarch64-none-elf cross
compiler to ensure that the original fix still works on AArch64.

OK?

Thanks,
James

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

	PR rtl-optimization/78561
	* config/rs6000/rs6000.c (rs6000_reg_live_or_pic_offset_p) Use
	constant_pool_empty_p in place of 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.
	* output.h (get_pool_size_upper_bound): Delete.
	(constant_pool_empty_p): New.
	* varasm.c (get_pool_size_upper_bound): Delete
	(constant_pool_empty_p): New.

Comments

Segher Boessenkool Dec. 7, 2016, 11:56 a.m. UTC | #1 
Hi James,

On Wed, Dec 07, 2016 at 10:31:14AM +0000, James Greenhalgh wrote:
> We always keep the entries in the constant pool after we first see them, and

> we figure out what gets used in the constant pool by sweeping through

> looking for referenced entries. That means we can always answer the

> question "has the constant pool ever had entries?" by checking if there

> is anything at all in there.

> 

> So that's what this patch does.

> 

> Bootstrapped on gcc110 (powerpc64-none-linux-gnu) configured with

> --enable-languages=all,ada,go,obj-c++ with no issues, Segher has also

> tested it with no issues, and I've tested an aarch64-none-elf cross

> compiler to ensure that the original fix still works on AArch64.

> 

> OK?


The rs6000 changes are okay for trunk.  The rest I cannot approve (but
it does look fine).  Thanks,


Segher


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

> 

> 	PR rtl-optimization/78561

> 	* config/rs6000/rs6000.c (rs6000_reg_live_or_pic_offset_p) Use


(Missing colon here)

> 	constant_pool_empty_p in place of 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.

> 	* output.h (get_pool_size_upper_bound): Delete.

> 	(constant_pool_empty_p): New.

> 	* varasm.c (get_pool_size_upper_bound): Delete

> 	(constant_pool_empty_p): New.
Bernd Schmidt Dec. 7, 2016, 1:55 p.m. UTC | #2 
On 12/07/2016 12:56 PM, Segher Boessenkool wrote:
> On Wed, Dec 07, 2016 at 10:31:14AM +0000, James Greenhalgh wrote:

>> We always keep the entries in the constant pool after we first see them, and

>> we figure out what gets used in the constant pool by sweeping through

>> looking for referenced entries. That means we can always answer the

>> question "has the constant pool ever had entries?" by checking if there

>> is anything at all in there.

>>

>> So that's what this patch does.

>>

>> Bootstrapped on gcc110 (powerpc64-none-linux-gnu) configured with

>> --enable-languages=all,ada,go,obj-c++ with no issues, Segher has also

>> tested it with no issues, and I've tested an aarch64-none-elf cross

>> compiler to ensure that the original fix still works on AArch64.

>>

>> OK?

>

> The rs6000 changes are okay for trunk.  The rest I cannot approve (but

> it does look fine).  Thanks,


Ok for the rest.


Bernd
diff mbox

Patch

diff --git a/gcc/config/rs6000/rs6000.c b/gcc/config/rs6000/rs6000.c
index 59bd3fe..b75a290 100644
--- a/gcc/config/rs6000/rs6000.c
+++ b/gcc/config/rs6000/rs6000.c
@@ -25460,7 +25460,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_upper_bound ()))
+	      || !constant_pool_empty_p ()))
 	return true;
 
       if ((DEFAULT_ABI == ABI_V4 || DEFAULT_ABI == ABI_DARWIN)
@@ -26266,7 +26266,7 @@  rs6000_stack_info (void)
 #ifdef TARGET_RELOCATABLE
       || (DEFAULT_ABI == ABI_V4
 	  && (TARGET_RELOCATABLE || flag_pic > 1)
-	  && get_pool_size_upper_bound () != 0)
+	  && !constant_pool_empty_p ())
 #endif
       || rs6000_ra_ever_killed ())
     info->lr_save_p = 1;
@@ -28044,7 +28044,7 @@  rs6000_emit_prologue (void)
 
       /* With -mminimal-toc we may generate an extra use of r2 below.  */
       if (TARGET_TOC && TARGET_MINIMAL_TOC
-	  && get_pool_size_upper_bound () != 0)
+	  && !constant_pool_empty_p ())
 	cfun->machine->r2_setup_needed = true;
     }
 
@@ -28900,7 +28900,7 @@  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_upper_bound () != 0)
+	   && !constant_pool_empty_p ())
 	  || (DEFAULT_ABI == ABI_V4
 	      && (flag_pic == 1 || (flag_pic && TARGET_SECURE_PLT))
 	      && df_regs_ever_live_p (RS6000_PIC_OFFSET_TABLE_REGNUM))))
@@ -34967,7 +34967,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_upper_bound () != 0 || crtl->profile)
+      && (!constant_pool_empty_p () || crtl->profile)
       && uses_TOC ())
     {
       char buf[256];
@@ -37453,7 +37453,7 @@  rs6000_can_eliminate (const int from, const int to)
 	  ? ! frame_pointer_needed
 	  : from == RS6000_PIC_OFFSET_TABLE_REGNUM
 	    ? ! TARGET_MINIMAL_TOC || TARGET_NO_TOC
-		|| get_pool_size_upper_bound () == 0
+		|| constant_pool_empty_p ()
 	    : true);
 }
 
@@ -38990,7 +38990,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_upper_bound () != 0)
+      && !constant_pool_empty_p ())
     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 7186dc1..6c99381 100644
--- a/gcc/output.h
+++ b/gcc/output.h
@@ -287,11 +287,10 @@  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 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);
+/* Return TRUE if and only if the constant pool has no entries.  Note
+   that even entries we might end up choosing not to emit are counted
+   here, so there is the potential for missed optimizations.  */
+extern bool constant_pool_empty_p (void);
 
 extern rtx_insn *peephole (rtx_insn *);
 
diff --git a/gcc/varasm.c b/gcc/varasm.c
index f3cd70a..5b15847 100644
--- a/gcc/varasm.c
+++ b/gcc/varasm.c
@@ -3808,12 +3808,14 @@  get_pool_mode (const_rtx addr)
   return SYMBOL_REF_CONSTANT (addr)->mode;
 }
 
-/* Return the size of the constant pool.  */
+/* Return TRUE if and only if the constant pool has no entries.  Note
+   that even entries we might end up choosing not to emit are counted
+   here, so there is the potential for missed optimizations.  */
 
-int
-get_pool_size_upper_bound (void)
+bool
+constant_pool_empty_p (void)
 {
-  return crtl->varasm.pool->offset;
+  return crtl->varasm.pool->first == NULL;
 }
 
 /* Worker function for output_constant_pool_1.  Emit assembly for X