| Message ID | 20150424192256.GO1751@tucnak.redhat.com |
|---|---|
| State | New |
| Headers | show |
On Fri, 24 Apr 2015, Jakub Jelinek wrote: > Hi! > > In vrp_visit_assignment_or_call we try to return SSA_PROP_VARYING > if update_value_range returned true and the new range is VR_VARYING, > but vrp_visit_phi_node fails to do that. > Another thing is that if update_value_range decides to > set_value_range_to_varying (old_vr); > it doesn't update new_vr, so the caller doesn't know the new vr > is varying (and calling get_value_range again sounds unnecessary). > > The following patch fixes it, bootstrapped/regtested on x86_64-linux and > i686-linux, ok for trunk and 5.2? > > 2015-04-24 Jakub Jelinek <jakub@redhat.com> > > PR tree-optimization/65875 > * tree-vrp.c (update_value_range): If in is_new case setting > old_vr to VR_VARYING, also set new_vr to it. > (vrp_visit_phi_node): Return SSA_PROP_VARYING instead of > SSA_PROP_INTERESTING if update_value_range returned true, > but new range is VR_VARYING. > > * gcc.c-torture/compile/pr65875.c: New test. > > --- gcc/tree-vrp.c.jj 2015-04-20 14:35:39.000000000 +0200 > +++ gcc/tree-vrp.c 2015-04-24 18:10:41.321367440 +0200 > @@ -892,7 +892,12 @@ update_value_range (const_tree var, valu > UNDEFINED or from VARYING. */ > if (new_vr->type == VR_UNDEFINED > || old_vr->type == VR_VARYING) > - set_value_range_to_varying (old_vr); > + { > + BITMAP_FREE (new_vr->equiv); > + set_value_range_to_varying (old_vr); > + set_value_range_to_varying (new_vr); > + return true; Actually we didn't change anything here (old_vr->type is VARYING already, so we shouldn't even have visited the stmt defining the SSA name again... eventually your fix below fixes that. So I'd prefer to simply drop the hunk... (you might want to do some digging which rev. introduced it and if it came with a testcase...). Otherwise ok. Thanks, Richard. > + } > else > set_value_range (old_vr, new_vr->type, new_vr->min, new_vr->max, > new_vr->equiv); > @@ -8941,6 +8946,9 @@ update_range: > fprintf (dump_file, "\n"); > } > > + if (vr_result.type == VR_VARYING) > + return SSA_PROP_VARYING; > + > return SSA_PROP_INTERESTING; > } > > --- gcc/testsuite/gcc.c-torture/compile/pr65875.c.jj 2015-04-24 18:20:47.650595581 +0200 > +++ gcc/testsuite/gcc.c-torture/compile/pr65875.c 2015-04-24 18:20:30.000000000 +0200 > @@ -0,0 +1,24 @@ > +/* PR tree-optimization/65875 */ > + > +int a, b, c, d, e, f, g; > + > +void > +foo (void) > +{ > + long h = 0, i; > + if (g < 0) > + i = -g; > + for (; b;) > + for (; c;) > + if (e) > + h = 1; > + for (; f;) > + if (a) > + break; > + if (h > i) > + while (h > i) > + { > + d = 0; > + h--; > + } > +} > > Jakub > >
On Mon, Apr 27, 2015 at 10:05:21AM +0200, Richard Biener wrote: > > --- gcc/tree-vrp.c.jj 2015-04-20 14:35:39.000000000 +0200 > > +++ gcc/tree-vrp.c 2015-04-24 18:10:41.321367440 +0200 > > @@ -892,7 +892,12 @@ update_value_range (const_tree var, valu > > UNDEFINED or from VARYING. */ > > if (new_vr->type == VR_UNDEFINED > > || old_vr->type == VR_VARYING) > > - set_value_range_to_varying (old_vr); > > + { > > + BITMAP_FREE (new_vr->equiv); > > + set_value_range_to_varying (old_vr); > > + set_value_range_to_varying (new_vr); > > + return true; > > Actually we didn't change anything here (old_vr->type is VARYING already, > so we shouldn't even have visited the stmt defining the SSA name again... > eventually your fix below fixes that. On the testcase, old_vr wasn't actually VARYING, but new_vr was UNDEFINED (a result of intersecting disjoint ranges). While for old_vr->type == VR_VARYING I agree we shouldn't have been called on this. And turning a VR_RANGE into VR_UNDEFINED is going in the wrong direction in the lattice. Jakub
On Mon, 27 Apr 2015, Jakub Jelinek wrote: > On Mon, Apr 27, 2015 at 10:05:21AM +0200, Richard Biener wrote: > > > --- gcc/tree-vrp.c.jj 2015-04-20 14:35:39.000000000 +0200 > > > +++ gcc/tree-vrp.c 2015-04-24 18:10:41.321367440 +0200 > > > @@ -892,7 +892,12 @@ update_value_range (const_tree var, valu > > > UNDEFINED or from VARYING. */ > > > if (new_vr->type == VR_UNDEFINED > > > || old_vr->type == VR_VARYING) > > > - set_value_range_to_varying (old_vr); > > > + { > > > + BITMAP_FREE (new_vr->equiv); > > > + set_value_range_to_varying (old_vr); > > > + set_value_range_to_varying (new_vr); > > > + return true; > > > > Actually we didn't change anything here (old_vr->type is VARYING already, > > so we shouldn't even have visited the stmt defining the SSA name again... > > eventually your fix below fixes that. > > On the testcase, old_vr wasn't actually VARYING, but new_vr was UNDEFINED > (a result of intersecting disjoint ranges). While for old_vr->type == > VR_VARYING I agree we shouldn't have been called on this. > > And turning a VR_RANGE into VR_UNDEFINED is going in the wrong direction > in the lattice. Ah, I misread the || for a &&. The patch is ok with just dropping the == VR_VARYING check. Thanks, Richard.
--- gcc/tree-vrp.c.jj 2015-04-20 14:35:39.000000000 +0200 +++ gcc/tree-vrp.c 2015-04-24 18:10:41.321367440 +0200 @@ -892,7 +892,12 @@ update_value_range (const_tree var, valu UNDEFINED or from VARYING. */ if (new_vr->type == VR_UNDEFINED || old_vr->type == VR_VARYING) - set_value_range_to_varying (old_vr); + { + BITMAP_FREE (new_vr->equiv); + set_value_range_to_varying (old_vr); + set_value_range_to_varying (new_vr); + return true; + } else set_value_range (old_vr, new_vr->type, new_vr->min, new_vr->max, new_vr->equiv); @@ -8941,6 +8946,9 @@ update_range: fprintf (dump_file, "\n"); } + if (vr_result.type == VR_VARYING) + return SSA_PROP_VARYING; + return SSA_PROP_INTERESTING; } --- gcc/testsuite/gcc.c-torture/compile/pr65875.c.jj 2015-04-24 18:20:47.650595581 +0200 +++ gcc/testsuite/gcc.c-torture/compile/pr65875.c 2015-04-24 18:20:30.000000000 +0200 @@ -0,0 +1,24 @@ +/* PR tree-optimization/65875 */ + +int a, b, c, d, e, f, g; + +void +foo (void) +{ + long h = 0, i; + if (g < 0) + i = -g; + for (; b;) + for (; c;) + if (e) + h = 1; + for (; f;) + if (a) + break; + if (h > i) + while (h > i) + { + d = 0; + h--; + } +}