[PATCH] D12170: Const propagatin after hitting assume bugfix

[Daniel Berlin via llvm-commits]

Sorry, i was a bit grumpy :)
At this point I think we both know the state of the world and agree we
aren't going to solve this set of problems in this set of patches.

So let's approve this (assuming there are no comments on the patch
itself) and move on :)


On Mon, Aug 24, 2015 at 6:06 PM, Nick Lewycky <nlewycky at google.com> wrote:
> On 24 August 2015 at 17:38, Daniel Berlin <dberlin at dberlin.org> wrote:
>>
>> On Mon, Aug 24, 2015 at 5:06 PM, Nick Lewycky <nlewycky at google.com> wrote:
>> > On 20 August 2015 at 19:31, Daniel Berlin via llvm-commits
>> > <llvm-commits at lists.llvm.org> wrote:
>> >>
>> >> dberlin added a comment.
>> >>
>> >> I assume you are hitting this in switch statements or something
>> >> similar.
>> >> I triggered the same in NewGVN.
>> >>
>> >> Note that the comment in dominates is simply wrong.
>> >> I discussed with this Rafael (who added this). The callers can, in
>> >> fact,
>> >> do *nothing* different than what the function would have to do, and in
>> >> fact,
>> >> would have to duplicate a ton of complicated critical edge logic that
>> >> Dominates handles.  That seems really bad to me.
>> >>
>> >> Nowhere else in LLVM do we *assert* because a very dumb caller may
>> >> cause a
>> >> performance problem.
>> >> The right fix here is to  simply remove the assert, and make sure your
>> >> caller is smart about not calling this unnecessarily.
>> >
>> >
>> > I'm not sure I follow.
>> >
>> > The point of the function that's already there is to query "does this
>> > *edge*
>> > dominate that block"?
>>
>> > When the edge is non-singular, it certainly doesn't
>> > (you can take the other edge between the same two blocks)
>>
>> So wait.
>> Let's stop here for a second.
>>
>> You believe dominance is well defined on edges, rather than vertices?
>>
>> I'm not aware of any definition of dominance on graphs that talks
>> about edges.  Can you point me at any literature that has a sound
>> definition of edge-dominance?
>
>
> Fair point. I'm not familiar with any such literature.
>
> I think the intent of this function is clear: this edge E dominates block B
> if and only if edge E must be taken to arrive at block B. That is -- edge E
> and not any other edge -- such as other edges between the same pair of
> blocks.
>
> I haven't thought through whether we end up with all the same things we
> expect out of dominance like edge-idom and edge-domtrees.
>
>> Dominance is normally defined in terms of vertices.
>> That means, if i have a billion edges directly between node A to node
>> B, they all have the same dominance properties.
>> Period.
>> You can still ask about a control flow edge. It is equivalent to does
>> source dominate end.
>
>
>> (This function, AFAIK, is trying to work around critical edge
>> splitting. Which is also side-humorous in that not splitting critical
>> edges provably causes GVN to miss significant redundancies, even
>> though it "solves" PRE's problem of not wanting to have to pre-split
>> edges)
>>
>> But if we've come up with some  definition of edge-dominance, i'd love
>> to see how it's built and it's soundness, because the comments say
>> nothing about what it means :)
>>
>> "Removing the assert may be the right thing to do, but it doesn't save
>> us from needing this patch."
>>
>> This patch essentially changes things to sometimes use edge-dominance,
>> and sometimes not.
>> Can you explain, in detail, why you believe this is the right thing?
>> I don't see it.
>
>
> Sure!
>
> As of today, the entirety of GVN learns new facts only along edges, based on
> which edge was taken. It only learns that %cmp is true because we branch to
> the %cond.true block in the true case. You're already familiar with the
> switch case, where we switch(i) and case i32 1 goes to %bb and case i32 2
> goes to %bb. The problem is that if you go from the switch to %bb, then we
> know neither whether %i is i32 1 nor i32 2.
>
> When looking at assumes, we aren't learning anything new based on which edge
> is chosen. Instead, we learn something mid-block, and that fact carries into
> all dominated blocks. Which edge we go at the bottom of our block just
> doesn't matter. Unlike every other case our GVN handles, what fact we know
> does not depend on which edge we take. That is the essential difference.
>
> Note that there's a swath of other opportunities like this one, which our
> GVN currently doesn't handle for the same reason; we don't handle "div x, y"
> implies y is non-zero, "x = add nsw y, z" implies that x sgt y, load with
> alignment implies certain bits of the pointer, etc. But we aren't ready to
> rewrite all of GVN to be Instruction* based instead of using edges, at least
> not in this patch.
>
>> It looks clearly wrong to me.  It looks like "maybe we can get away
>> with avoiding this assert in this case, but we are trying to work
>> around the problem instead of fixing it".
>>
>> Then again, i also don't think trying to use dominance to approximate
>> control regions is right either, so what do i know :)
>
>
> Yeah, this is another good point, but I'm going to defer to existing
> practice.