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

Wed Nov 9 11:45:49 PST 2016

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

On Wed, Nov 9, 2016 at 11:31 AM, David Majnemer <david.majnemer at gmail.com>
wrote:

>
>
> On Wed, Nov 9, 2016 at 11:16 AM, Davide Italiano via llvm-commits <
> llvm-commits at lists.llvm.org> wrote:
>
>> On Wed, Nov 9, 2016 at 9:49 AM, Daniel Berlin via llvm-commits
>> <llvm-commits at lists.llvm.org> wrote:
>> > Actually, digging further into it,  I don't understand how this fixes
>> it at
>> > all, and your test does nothing because it's all CHECK's :-)
>> >
>> > The example given in the bug report is or true, undef
>> >
>> > ResolveUndefs already does the right thing for this, and does not
>> consider
>> > the result undef.
>> >
>> > The propagation order makes it impossible for that to happen, because in
>> > reality, undef is a set of lattice values but not represented in the
>> > lattice, and resolveUndefs is trying to figure out the lattice values
>> that
>> > work.
>> >
>> > That's ... never going to work, as you've discovered (and eli suspects
>> on
>> > the bug report).
>> >
>> > I suspect pretty much every part of markForcedConstant is vulnerable to
>> this
>> > undef different circumstances.
>> >
>> > Realistically, undef needs to be an actual lattice value lower than
>> unknown
>> >
>> > That way, or undef, true comes out true during *solving*.
>> > The parts of resolveundefs handling this need to be made part of the
>> meet
>> > operation
>> >
>> > This handles the case where undef must be constrained (IE or undef,
>> true).
>> >
>> > After solving is finished, anything left as undef can then be filled in
>> > unconstrained, because you are guaranteed it is not forced to a certain
>> > value (or that your solver sucks :P)
>> >
>> > (note that in the freeze/poison proposal, this should just work because
>> the
>> > freezes will be different ssa names)
>> >
>>
>> Uh, thanks. Out of curiosity, do you happen to know why undef wasn't
>> represented as a lattice value from the beginning (and instead we end
>> up trying values that match up?) Is there a particular reason (e.g.
>> efficiency)? I can't think of one.
>>
>
> Our SCCP code dates back to 2001, undef arrived in 2004.
>
>
>> --
>> Davide
>> _______________________________________________
>> llvm-commits mailing list
>> llvm-commits at lists.llvm.org
>> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20161109/ead79663/attachment.html>