<div dir="ltr">(This code is pretty messy, so even that turns out to be a bit more work than i have time for ATM).<div><br></div></div><div class="gmail_extra"><br><div class="gmail_quote">On Wed, Nov 9, 2016 at 1:00 PM, Daniel Berlin <span dir="ltr"><<a href="mailto:dberlin@dberlin.org" target="_blank">dberlin@dberlin.org</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr">Okay.<div>I may make the minimal fix to just split undef and unknown, and change the resolver to use undef.</div><div>It should just work, even if it's not time-optimal.</div><div><br></div></div><div class="HOEnZb"><div class="h5"><div class="gmail_extra"><br><div class="gmail_quote">On Wed, Nov 9, 2016 at 12:42 PM, Davide Italiano <span dir="ltr"><<a href="mailto:davide@freebsd.org" target="_blank">davide@freebsd.org</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="m_-9016829307074025613HOEnZb"><div class="m_-9016829307074025613h5">On Wed, Nov 9, 2016 at 11:45 AM, Daniel Berlin <<a href="mailto:dberlin@dberlin.org" target="_blank">dberlin@dberlin.org</a>> wrote:<br>
> Right.<br>
> Like a lot of things, it was probably reasonably assumed this would just<br>
> work.<br>
><br>
> It's take a lot of years to find cases where that's not true.<br>
><br>
> While it would have been super nice if, when this was added, folks sat down<br>
> and thought very hard about the theory behind it and prove it would work,<br>
> that doesn't always (or often :P) happen.<br>
><br>
><br>
> Here, it's, sadly, pretty easy to prove it can't work:<br>
><br>
> The solver follows chains until the lattice values stop changing.<br>
> The end state is one where things have the "best" lattice value they can.<br>
><br>
> Because we use unknown to mean two different things (unprocessed, and<br>
> undef), it means it does not resolve some things.<br>
> That by itself is okay.<br>
><br>
> However, what happens next is two fold:<br>
><br>
> 1. At the point the solver finishes, it expects unknown means undef<br>
> 2. The iteration order of the resolver is not the same as the solver (IE it<br>
> walks basic blocks, it does not propagate from defs to uses).<br>
><br>
> These two things combine.<br>
><br>
> Because it iterates in an order not guaranteed to process defs before uses<br>
> (because it's iterating over *basic blocks* and in *function order* instead<br>
> of *the ssa graph*, and in *def->use* order) it hits the branch condition<br>
> use before the branch condition def.<br>
> It then uses #1 to assume that unknown there means undef. But it could just<br>
> mean "the solver could not resolve this", which is *not* the same as undef.<br>
><br>
> For those having trouble following this:<br>
><br>
> The solver processs<br>
><br>
> %a = or true, undef<br>
> <in some other basic block><br>
> branch %a<br>
><br>
> in order of: %a, branch<br>
><br>
> The resolver<br>
> may process it as<br>
> branch, %a.<br>
><br>
> At the point it sees the branch, it thinks anything marked "unknown" (like<br>
> %a") is really "undef".<br>
> It's not.<br>
> It's really "unknown" :)<br>
><br>
> If the resolver processed it as<br>
> %a, branch<br>
> it would do the "right" thing.<br>
><br>
> So if you fixed the iteration order, you'd have a much better chance of<br>
> making it work in the face of #1, but i'm pretty sure, at a minimum, where<br>
> there are cycles, you can still prove it will break.<br>
><br>
> IMHO, it's easier to just distinguish between unknown and undef, *and* move<br>
> the real work back into the meet/etc of the propagation engine, so that the<br>
> iteration order is fixed as well.<br>
><br>
> (so, yeah, you could also fix this bug by making resolve undefs do the same<br>
> exact propagation ordering as the solver, *and* by fixing #1. But i'm<br>
> suggesting it's easier to just integrate most of this work into the solver<br>
> instead of trying to have two parallel pieces of code have a lockstep<br>
> ordering of propagation).<br>
><br>
><br>
<br>
</div></div>Thank you very much for the detailed explanation. For now I'm tackling<br>
GVN so it's unlikely I'll have time to look at this anytime soon.<br>
I opened a bug <a href="https://llvm.org/bugs/show_bug.cgi?id=30966" rel="noreferrer" target="_blank">https://llvm.org/bugs/show_bug<wbr>.cgi?id=30966</a> in case<br>
somebody on the list is interested (and so that I won't forget =)<br>
<br>
--<br>
Davide<br>
</blockquote></div><br></div>
</div></div></blockquote></div><br></div>