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

[Daniel Berlin via llvm-commits]

(This code is pretty messy, so even that turns out to be a bit more work
than i have time for ATM).


On Wed, Nov 9, 2016 at 1:00 PM, Daniel Berlin <dberlin at dberlin.org> wrote:

> 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/20161110/6a81768a/attachment.html>