[PATCH] D33584: Remove a quadratic behavior in assert-enabled builds

[Daniel Berlin via Phabricator via llvm-commits]

dberlin added a comment.

In https://reviews.llvm.org/D33584#766122, @sanjoy wrote:

> In https://reviews.llvm.org/D33584#766056, @anemet wrote:
>
> > Removed the asserts.  As Danny put it:
> >
> > "Given LLVM defines edge dominance in a way that means non-unique edges never dominate their end, this is a waste of time."
> >
> > In other words, there is no need for an assert here since it's not the case
> >  that the answer would be wrong or would take more time to compute than for
> >  unique edges.  It's simply that the answer would always be non-dominance by
> >  how domininance of edges is defined.
>
>
> I'm not sure I agree with this.  To be clear, say we have:
>
>   bb0:
>     br i1 undef, label %bb1, label %bb1
>  
>   bb1;
>     ...
>
>
> are you suggesting that `dominates([bb0->bb1], bb1)` will be false anyway, and there is no specific need to check `isSingleEdge()` at all?


yes, the current callers will all do the equivalent of returning  false.

See isReachableOnlyByThisEdge, and the GVN case pointed out earlier.

> That does not seem to be the case -- I think `DominatorTree::dominates([bb0->bb1], bb1)` will return true.

This is the part where i said the patch is likely broken.
The thing we did before the assert was to return false if !singleEdge.

Hence my comment that "i'm not sure the pred loop will check what we need to have the same behavior we used to"

> Today if the `bb0` -> `bb1` edge dominates some use of `%cond` then said use can be replaced with `i1 true`, but with your change that will no longer hold.

Today, any caller that asks if edge bb0->bb1 dominates some use of cond, it will assert :)

It will not return true ;)
Previous to the assert, we returned false.
This is why it says:
"// Assert that we have a single edge. We could handle them by simply
returning false. "

Now, you are right  that there are situation we *could* return true, but we wouldn't :)


https://reviews.llvm.org/D33584