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