| Message ID | 1dee97faa622a09dfd63f63e5484ca09ee70c3e5.1552671499.git.segher@kernel.crashing.org |
|---|---|
| State | New |
| Headers | show |
| Series | LRA: side_effects_p stmts' output is not invariant (PR89721) | expand |
On 2019-03-15 2:30 p.m., Segher Boessenkool wrote: > PR89721 shows LRA treating an unspec_volatile's result as invariant, > which of course isn't correct. This patch fixes it. Segher, thank you for fixing this. The patch is ok to commit. > Question. Is side_effects_p the correct check? Or do we want to > allow PRE_INC etc. here? What about CALL? > No, we don't want INC/DEC. Inheritance helps to reuse some reloaded values which are expensive for calculation. So if we have two INCs, we will have only one, which is wrong. The same is for calls. I believe we can ignore calls here because LRA does not reload calls (call result reg). Actually I believe the same is true for INC/DEC (we can reload INC/DEC regs but not INC/DEC themselves). > Segher > > > 2019-04-15 Segher Boessenkool <segher@kernel.crashing.org> > > PR rtl-optimization/89721 > * lra-constraints (invariant_p): Return false if side_effects_p holds. > > --- > gcc/lra-constraints.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/gcc/lra-constraints.c b/gcc/lra-constraints.c > index afbd5d0..24f11ed 100644 > --- a/gcc/lra-constraints.c > +++ b/gcc/lra-constraints.c > @@ -5839,6 +5839,9 @@ invariant_p (const_rtx x) > enum rtx_code code; > int i, j; > > + if (side_effects_p (x)) > + return false; > + > code = GET_CODE (x); > mode = GET_MODE (x); > if (code == SUBREG)
Hi! On Fri, Mar 15, 2019 at 04:25:01PM -0400, Vladimir Makarov wrote: > On 2019-03-15 2:30 p.m., Segher Boessenkool wrote: > >PR89721 shows LRA treating an unspec_volatile's result as invariant, > >which of course isn't correct. This patch fixes it. > Segher, thank you for fixing this. The patch is ok to commit. Thanks, done. Is this okay for backports to 8 and 7 as well? After a while of course. > >Question. Is side_effects_p the correct check? Or do we want to > >allow PRE_INC etc. here? What about CALL? > > > No, we don't want INC/DEC. Inheritance helps to reuse some reloaded > values which are expensive for calculation. So if we have two INCs, we > will have only one, which is wrong. The same is for calls. And we could handle it, but that requires code that isn't there yet, and is it worth it anyway. Okay :-) Segher
On Fri, Mar 15, 2019 at 05:14:48PM -0500, Segher Boessenkool wrote: > On Fri, Mar 15, 2019 at 04:25:01PM -0400, Vladimir Makarov wrote: > > On 2019-03-15 2:30 p.m., Segher Boessenkool wrote: > > >PR89721 shows LRA treating an unspec_volatile's result as invariant, > > >which of course isn't correct. This patch fixes it. > > Segher, thank you for fixing this. The patch is ok to commit. > > Thanks, done. Is this okay for backports to 8 and 7 as well? After a > while of course. I lost track of this. Is it okay to backport to 8 and 7? Segher
On 10/17/19 2:09 PM, Segher Boessenkool wrote: > On Fri, Mar 15, 2019 at 05:14:48PM -0500, Segher Boessenkool wrote: >> On Fri, Mar 15, 2019 at 04:25:01PM -0400, Vladimir Makarov wrote: >>> On 2019-03-15 2:30 p.m., Segher Boessenkool wrote: >>>> PR89721 shows LRA treating an unspec_volatile's result as invariant, >>>> which of course isn't correct. This patch fixes it. >>> Segher, thank you for fixing this. The patch is ok to commit. >> Thanks, done. Is this okay for backports to 8 and 7 as well? After a >> while of course. > I lost track of this. Is it okay to backport to 8 and 7? > > Yes, sure it is ok for 8 and 7 too. Thank you, Segher.
diff --git a/gcc/lra-constraints.c b/gcc/lra-constraints.c index afbd5d0..24f11ed 100644 --- a/gcc/lra-constraints.c +++ b/gcc/lra-constraints.c @@ -5839,6 +5839,9 @@ invariant_p (const_rtx x) enum rtx_code code; int i, j; + if (side_effects_p (x)) + return false; + code = GET_CODE (x); mode = GET_MODE (x); if (code == SUBREG)