Patchwork Followup for reg_equiv_invariant patch: Fix PR39871

login
register
mail settings
Submitter Bernd Schmidt
Date June 14, 2010, 10:44 p.m.
Message ID <4C16B0B4.6080604@codesourcery.com>
Download mbox | patch
Permalink /patch/55594/
State New
Headers show

Comments

Bernd Schmidt - June 14, 2010, 10:44 p.m.
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.
Joern Rennecke - June 15, 2010, 2:35 a.m.
Quoting Bernd Schmidt <bernds@codesourcery.com>:

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

What happens if we have a (plus (REG:SI SP_REG) (symbol_ref foo)) ?
Or will this never happen for flag_pic?
Jeff Law - June 15, 2010, 4:34 a.m.
On 06/14/10 20:35, Joern Rennecke wrote:
> Quoting Bernd Schmidt <bernds@codesourcery.com>:
>
>> Joern, any comments - do you recall any reason why this change would
>> have been intentional?
>
> What happens if we have a (plus (REG:SI SP_REG) (symbol_ref foo)) ?
> Or will this never happen for flag_pic?
I can't see how this would ever be valid when flag_pic.

Jeff
Joern Rennecke - June 15, 2010, 6 a.m.
Quoting Jeff Law <law@redhat.com>:

> On 06/14/10 20:35, Joern Rennecke wrote:
>> Quoting Bernd Schmidt <bernds@codesourcery.com>:
>>
>>> Joern, any comments - do you recall any reason why this change would
>>> have been intentional?
>>
>> What happens if we have a (plus (REG:SI SP_REG) (symbol_ref foo)) ?
>> Or will this never happen for flag_pic?
> I can't see how this would ever be valid when flag_pic.

The question is if such invalid expressions might be in notes at these
points; if that might be the case, the code needs to reject them.

function_invariant_p will accept them.  An argument can be made that it
should reject them, but should it then be renamed to
function_invariant_and_suitable_for_pic_if_the_latter_matters_p or
rematerializable_invariant_p ?
The reason why I missed the issue with constants passed to  
LEGITIMATE_PIC_OPERAND_P back in 1998 was that I assumed the macro name
was descriptive and didn't check the documentation in tm.texi.
Paolo Bonzini - June 15, 2010, 9:37 a.m.
My reload-fu is approximately zero, but...

On 06/15/2010 12:44 AM, Bernd Schmidt wrote:
> +	  /* If flag_pic and we have constant, verify it's legitimate.  */
> +	  if (!CONSTANT_P (x)
> +	      || !flag_pic
> +	      || LEGITIMATE_PIC_OPERAND_P (x))

This comment is not adding anything to the if condition, and maybe could 
be replaced by the comment inside the braces.

>   	    {
>   	      /* 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))

... and this comment becomes out-of-date as there's no reference anymore 
to function_invariant_p in the code.

Paolo
Joern Rennecke - June 15, 2010, 10:50 a.m.
Quoting Paolo Bonzini <bonzini@gnu.org>:

>> +	  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))
>
> ... and this comment becomes out-of-date as there's no reference
> anymore to function_invariant_p in the code.

It becomes more relevant because the note likely contains an invariant
tested before with function_invariant_p.
Bernd Schmidt - June 15, 2010, 10:55 a.m.
On 06/15/2010 08:00 AM, Joern Rennecke wrote:
> Quoting Jeff Law <law@redhat.com>:
> 
>> On 06/14/10 20:35, Joern Rennecke wrote:
>>> Quoting Bernd Schmidt <bernds@codesourcery.com>:
>>>
>>>> Joern, any comments - do you recall any reason why this change would
>>>> have been intentional?
>>>
>>> What happens if we have a (plus (REG:SI SP_REG) (symbol_ref foo)) ?
>>> Or will this never happen for flag_pic?
>> I can't see how this would ever be valid when flag_pic.
> 
> The question is if such invalid expressions might be in notes at these
> points; if that might be the case, the code needs to reject them.
> 
> function_invariant_p will accept them.

I guess we can change that and not lose anything.


Bernd

Patch

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