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

[Davide Italiano via llvm-commits]

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