[PATCH] D26432: [SCCP] Force unknown branches to overdefined

[Daniel Berlin via llvm-commits]

Okay.
I may make the minimal fix to just split undef and unknown, and change the
resolver to use undef.
It should just work, even if it's not time-optimal.


On Wed, Nov 9, 2016 at 12:42 PM, Davide Italiano <davide at freebsd.org> wrote:

> On Wed, Nov 9, 2016 at 11:45 AM, Daniel Berlin <dberlin at dberlin.org>
> wrote:
> > Right.
> > Like a lot of things, it was probably reasonably assumed this would just
> > work.
> >
> > It's take a lot of years to find cases where that's not true.
> >
> > While it would have been super nice if, when this was added, folks sat
> down
> > and thought very hard about the theory behind it and prove it would work,
> > that doesn't always (or often :P) happen.
> >
> >
> > Here, it's, sadly, pretty easy to prove it can't work:
> >
> > The solver follows chains until the lattice values stop changing.
> > The end state is one where things have the "best" lattice value they can.
> >
> > Because we use unknown to mean two different things (unprocessed, and
> > undef), it means it does not resolve some things.
> > That by itself is okay.
> >
> > However, what happens next is two fold:
> >
> > 1. At the point the solver finishes, it expects unknown means undef
> > 2. The iteration order of the resolver is not the same as the solver (IE
> it
> > walks basic blocks, it does not propagate from defs to uses).
> >
> > These two things combine.
> >
> > Because it iterates in an order not guaranteed to process defs before
> uses
> > (because it's iterating over *basic blocks* and in *function order*
> instead
> > of *the ssa graph*, and in *def->use* order) it hits the branch condition
> > use before the branch condition def.
> > It then uses #1 to assume that unknown there means undef. But it could
> just
> > mean "the solver could not resolve this", which is *not* the same as
> undef.
> >
> > For those having trouble following this:
> >
> > The solver processs
> >
> > %a = or true, undef
> > <in some other basic block>
> > branch %a
> >
> > in order of: %a, branch
> >
> > The resolver
> > may process it as
> > branch, %a.
> >
> > At the point it sees the branch, it thinks anything marked "unknown"
> (like
> > %a") is really "undef".
> > It's not.
> > It's really "unknown" :)
> >
> > If the resolver processed it as
> > %a, branch
> > it would do the "right" thing.
> >
> > So if you fixed the iteration order, you'd have a much better chance of
> > making it work in the face of #1, but i'm pretty sure, at a minimum,
> where
> > there are cycles, you can still prove it will break.
> >
> > IMHO, it's easier to just distinguish between unknown and undef, *and*
> move
> > the real work back into the meet/etc of the propagation engine, so that
> the
> > iteration order is fixed as well.
> >
> > (so, yeah, you could also fix this bug by making resolve undefs do the
> same
> > exact propagation ordering as the solver, *and* by fixing #1. But i'm
> > suggesting it's easier to just integrate most of this work into the
> solver
> > instead of trying to have two parallel pieces of code have a lockstep
> > ordering of propagation).
> >
> >
>
> Thank you very much for the detailed explanation. For now I'm tackling
> GVN so it's unlikely I'll have time to look at this anytime soon.
> I opened a bug https://llvm.org/bugs/show_bug.cgi?id=30966 in case
> somebody on the list is interested (and so that I won't forget =)
>
> --
> Davide
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20161109/8d031de0/attachment.html>