| Message ID | 4C16B0B4.6080604@codesourcery.com |
|---|---|
| State | New |
| Headers | show |
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?
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
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.
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
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.
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
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
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.