Re: [PATCH] Fix PR55011

[Michael Matz <matz at suse dot de>]

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.

> 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.

> > > 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.

> 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.

> 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.

> > > 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.


Ciao,
Michael.