[PATCH] D12170: Const propagatin after hitting assume bugfix

[Daniel Berlin via llvm-commits]

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?

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.

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