This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
Re: [PATCH] Fix PR55011
On Tue, 23 Oct 2012, Michael Matz wrote:
> Hi,
>
> On Tue, 23 Oct 2012, Richard Biener wrote:
>
> > > > > ... 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.
>
> Huh? It should yield VARYING, i.e. BOTTOM, not UNDEFINED, aka TOP.
> That's the whole point I'm trying to make. You're fixing up this very
> bug.
I suppose we can argue about this one. When using VARYING here
this VARYING can leak out from unreachable paths in the CFG.
> > > > 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 actually does what I say update_value_range should do. It _asserts_ a
> valid transition, and the fixup before is correctly marked with a ???
> comment about the improperty of the place of such fixing up.
>
> > It's just not convenient to repeat the adjustments everywhere.
>
> Sure. If the workers were correct there would be no need to do any
> adjustments.
>
> > > > 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.
>
> Hmm? See above.
>
> > > 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.
>
> And that's why the workers are not supposed to do that. They can't
> willy-nilly create new UNDEFINED/TOP lattice values. Somehow we have a
> disconnect in this discussion over some very basic property, let's try to
> clean this up first.
Well, consider 0 * VARYING. That's still [0, 0], not VARYING.
Workers should compute the best approximation for a range of
an operation, otherwise we cannot use them to build up operations
by pieces (like we do for -b).
> So, one question, are you claiming that a VRP worker like this:
>
> VR derive_new_range_from_operation (VR a, VR b)
>
> is _ever_ allowed to return UNDEFINED when a or b is something else than
> UNDEFINED? You seem to claim so AFAIU, but at the same time admit that
> this results in oscillation, and hence needs fixing up in the routine that
> uses the above result to install the lattice value in the map. I'm
> obviously saying that the above worker is not allowed to return UNDEFINED.
Yes. Do you say it never may return a RANGE if either a or b is
VARYING?
The whole point is that we have both derive_new_range_from_operation
and update_lattice_value. Only the latter knows how we iterate
and that this iteration restricts the values we may update the
lattice with. derive_new_range_from_operation is supposed to be
generally useful.
Richard.
--
Richard Biener <rguenther@suse.de>
SUSE / SUSE Labs
SUSE LINUX Products GmbH - Nuernberg - AG Nuernberg - HRB 16746
GF: Jeff Hawn, Jennifer Guild, Felix Imend