On Mon, 22 Oct 2012, Michael Matz wrote: > Hi, > > On Mon, 22 Oct 2012, Richard Biener wrote: > > > On Mon, 22 Oct 2012, Michael Matz wrote: > > > > > Hi, > > > > > > On Mon, 22 Oct 2012, Richard Biener wrote: > > > > > > > > > > > This fixes PR55011, it seems nothing checks for invalid lattice > > > > transitions in VRP, > > > > > > That makes sense, because the individual parts of VRP that produce new > > > ranges are supposed to not generate invalid transitions. So if anything > > > such checking should be an assert and the causes be fixed. > > > > No, the checking should be done in update_value_range > > Exactly. And that's the routine you're changing, but you aren't adding > checking, you silently "fix" invalid transitions. What I tried to say > is that the one calling update_value_range with new_vr being UNDEFINED is > wrong, and update_value_range shouldn't fix it, but assert, so that this > wrong caller may be fixed.
Callers do not look at the lattice value they change and it would not be convenient to do so in the various places. I re-word your complaint to "the function has a wrong name" then, which even I would not agree with. update_value_range is told to update the lattice value-range from lhs with the information from the given range. > > which copies the new VR over to the lattice. The job of that function > > is also to detect lattice changes. > > Sure, but not to fix invalid input. The input isn't invalid. The input cannot be put into the lattice just because that would be an invalid transition. > > > > so the following adds that > > > > > > It's a work around ... > > > > No. > > > > > > since we now can produce a lot more UNDEFINED than before > > > > > > ... for this. We should never "produce" UNDEFINED when the input wasn't > > > UNDEFINED already. > > > > Why? > > Because doing so _always_ means an invalid lattice transition. UNDEFINED > is TOP, anything not UNDEFINED is not TOP. So going from something to > UNDEFINED is always going upward the lattice and hence in the wrong > direction. Um, what do you mean with "input" then? Certainly intersecting [2, 4] and [6, 8] yields UNDEFINED. And the "input"s are not UNDEFINED. > > We shouldn't update the lattice this way, yes, but that is what > > the patch ensures. > > An assert ensures. A work around works around a problem. I say that the > problem is in those routines that produced the "new" UNDEFINED range in > the first place, and it's not update_value_range's job to fix that after > the fact. It is. See how CCPs set_lattice_vlaue adjusts the input as well. It's just not convenient to repeat the adjustments everywhere. > > The workers only compute a new value-range > > for a stmt based on input value ranges. > > And if they produce UNDEFINED when the input wasn't so, then _that's_ > where the bug is. See above. > > > > not doing so triggers issues. > > > > > > Hmm? > > > > It oscillates and thus never finishes. > > I'm not sure I understand. You claim that the workers have to produce > UNDEFINED from non-UNDEFINED in some cases, otherwise we oscillate? That > sounds strange. Or do you mean that we oscillate without your patch to > update_value_range? That I believe, it's the natural result of going a > lattice the wrong way, but I say that update_value_range is not the place > to silently fix invalid transitions. No, I mean that going up the lattice results in oscillation. richard.
