From patchwork Mon Jun 14 22:44:04 2010
Content-Type: text/plain; charset="utf-8"
MIME-Version: 1.0
Content-Transfer-Encoding: 7bit
Subject: Followup for reg_equiv_invariant patch: Fix PR39871
Date: Mon, 14 Jun 2010 12:44:04 -0000
From: Bernd Schmidt <bernds@codesourcery.com>
X-Patchwork-Id: 55594
Message-Id: <4C16B0B4.6080604@codesourcery.com>
To: GCC Patches <gcc-patches@gcc.gnu.org>
Cc: Joern Rennecke <amylaar@spamcop.net>

For flag_pic, reload currently disables the reg_equiv_invariant
machinary, which leads to poor code generation in PR39871 (a variant of
PR42500, 42502 and 40615 which were fixed by my earlier patch).

The problematic if statement looks like this:

	  if (! function_invariant_p (x)
	      || ! flag_pic
	      /* A function invariant is often CONSTANT_P but may
		 include a register.  We promise to only pass
		 CONSTANT_P objects to LEGITIMATE_PIC_OPERAND_P.  */
	      || (CONSTANT_P (x)
		  && LEGITIMATE_PIC_OPERAND_P (x)))

It has a bit of history.  Originally, we only supported REG_EQUIV for
constants, and it looked like this:

         if (note
#ifdef LEGITIMATE_PIC_OPERAND_P
             && (! CONSTANT_P (XEXP (note, 0)) || ! flag_pic
                 || LEGITIMATE_PIC_OPERAND_P (XEXP (note, 0)))
 #endif
             )

This simply avoids using constants that aren't LEGITIMATE_PIC_OPERAND_P.
 Then, Joern added the machinery that allows us to process equivalences
for things that are function_invariant_p, and I think this is where the
mistake happened:

 #ifdef LEGITIMATE_PIC_OPERAND_P
-             && (! CONSTANT_P (XEXP (note, 0)) || ! flag_pic
+             && (! function_invariant_p (XEXP (note, 0))
+                 || ! flag_pic
                  || LEGITIMATE_PIC_OPERAND_P (XEXP (note, 0)))
 #endif

This looks like an error - the patch replaces several occurrences of
CONSTANT_P with function_invariant_p in order to enable the extra
optimization, but here I think the test should simply have been left
alone; we deal with the various cases of equivalences inside this if
statement.

This also introduced a new bug: later on, someone else noticed that this
code could now pass non-constant objects to LEGITIMATE_PIC_OPERAND_P,
and added the extra CONSTANT_P check around that test, which gives us
the current version of the code.

The patch below simply restores this to essentially the original form.
Regression tested on arm-none-linux-gnueabi with my usual set of
multilibs (also bootstrapped and regression tested on i686-linux last
week IIRC, will retest before checking in).

Joern, any comments - do you recall any reason why this change would
have been intentional?


Bernd
PR rtl-optimization/39871
	* reload1.c (init_eliminable_invariants): For flag_pic, disable
	equivalences only for constants that aren't LEGITIMATE_PIC_OPERAND_P.
	* ira.c (find_reg_equiv_invariant_const): Likewise.


Index: reload1.c
===================================================================
--- reload1.c	(revision 160663)
+++ reload1.c	(working copy)
@@ -4151,13 +4151,10 @@ init_eliminable_invariants (rtx first, b
 	  if (i <= LAST_VIRTUAL_REGISTER)
 	    continue;
 
-	  if (! function_invariant_p (x)
-	      || ! flag_pic
-	      /* A function invariant is often CONSTANT_P but may
-		 include a register.  We promise to only pass
-		 CONSTANT_P objects to LEGITIMATE_PIC_OPERAND_P.  */
-	      || (CONSTANT_P (x)
-		  && LEGITIMATE_PIC_OPERAND_P (x)))
+	  /* If flag_pic and we have constant, verify it's legitimate.  */
+	  if (!CONSTANT_P (x)
+	      || !flag_pic
+	      || LEGITIMATE_PIC_OPERAND_P (x))
 	    {
 	      /* It can happen that a REG_EQUIV note contains a MEM
 		 that is not a legitimate memory operand.  As later
Index: ira.c
===================================================================
--- ira.c	(revision 160663)
+++ ira.c	(working copy)
@@ -1586,12 +1586,12 @@ find_reg_equiv_invariant_const (void)
 
 	  x = XEXP (note, 0);
 
-	  if (! function_invariant_p (x)
+	  if (! CONSTANT_P (x)
 	      || ! flag_pic
 	      /* A function invariant is often CONSTANT_P but may
 		 include a register.  We promise to only pass CONSTANT_P
 		 objects to LEGITIMATE_PIC_OPERAND_P.  */
-	      || (CONSTANT_P (x) && LEGITIMATE_PIC_OPERAND_P (x)))
+	      || LEGITIMATE_PIC_OPERAND_P (x))
 	    {
 	      /* It can happen that a REG_EQUIV note contains a MEM
 		 that is not a legitimate memory operand.  As later