[PATCH] D16100: Fix for two constant propagation problems in GVN with assume intrinsic instruction

[Daniel Berlin via llvm-commits]

On Tue, Jan 12, 2016 at 1:02 PM, Yuanrui Zhang <yuanrui.zhang at intel.com>
wrote:

> Ray added inline comments.
>
> ================
> Comment at: lib/Transforms/Scalar/GVN.cpp:2137
> @@ +2136,3 @@
> +/// If DominatesByEdge is false, it means that there might be multiple
> edges
> +/// from Root.Start to Root.End, in which case we need to pass Root.Start
> and
> +/// use a different implementation of replaceDominatedUsesWith to test.
> ----------------
> dberlin wrote:
> > If there are multiple edges to the same BB, we should not do the
> replacement at all (we lack post-dominance info necessary to be safe in
> that case).
> Note that, there is a condition about "multiple edges to the same BB":  it
> is from Root.Start to Root.End. That's when DominatesByEdge is set to
> false, and that's exactly the reason why DominatesByEdge was introduced as
> a parameter to this function.  See the example added before
>

This seems just wrong, for the reasons you've pointed out.

>
>
> I was trying to make the comments in more detail. But if it is confusing,
> we can improve it.
>
> ================
> Comment at: lib/Transforms/Scalar/GVN.cpp:2201
> @@ -2197,3 +2200,3 @@
>                ? replaceDominatedUsesWith(LHS, RHS, *DT, Root)
> -              : replaceDominatedUsesWith(LHS, RHS, *DT, Root.getEnd());
> +              : replaceDominatedUsesWith(LHS, RHS, *DT, Root.getStart());
>
> ----------------
> dberlin wrote:
> > If this problem only occurs with assume instructions, we shouldn't
> change the generic code.
> > If it's not a problem that only occur with assume instructions, we need
> > A. more testcases.
> > B. a more complete description of the problem that is happening and why
> this is the correct fix.
> >
> ===This is a general problem in GVN::propagateEquality with the following
> code:
>
> unsigned NumReplacements =
>           DominatesByEdge
>               ? replaceDominatedUsesWith(LHS, RHS, *DT, Root)
>               : replaceDominatedUsesWith(LHS, RHS, *DT, Root.getStart());
>
> When DominatesByEdge is TRUE, it passes the edge "Root" to
> replaceDominatedUsesWith, and we are fine. Since, if the edge "Root"
> dominates a USE, then Root.Start must dominates the USE.
>

Yes, it must, but that does not make it safe :)

If you propagate something through a switch statement, for example, it will
have different cases that go to the same block, but you can't replace
things in those blocks :)

THat is, root.getStart will dominate all the uses, but you can't do *any*
replacement.
root.getEnd will not dominate all the uses, only the ones actually in the
target edge end.

>
> When DominatesByEdge is FALSE, it passes the Root.End to
> replaceDominatedUsesWith, and we have a problem here.  Since, if Root.End
> dominates a USE, it does not mean that Root.Start dominates the USE. We
> show in the test case _Z1im below.
>

> bool RootDominatesEnd = DT.dominates(Root.Start(), Root.End()), then it
> can be used:
>

This will hit the multiple edge case, sadly.


> In fact, the simplest way is to
> 1) pass Root.Start  to replaceDominatedUsesWith,
> 2) then change replaceDominatedUsesWith(... BasicBlock* BB)  to replace
> the uses by the "END" of the BB.
> These two changes together will give exactly the same effect as when
> passing an edge, i.e. ,replaceDominatedUsesWith(.....   BasicBlockEdge
> Root).
>
> I'm not sure this is worth the confusion/cost.

This seems *super* complicated.


> Note that, currently GVN is the only caller of replaceDominatedUsesWith(
> ...,    BasicBlock* BB).  So, it is safe to change to use
> "properlyDominates".
>
>
It's safe, but confusing.

>
> ================
> Comment at: lib/Transforms/Utils/Local.cpp:1540
> @@ -1539,3 +1539,3 @@
>      auto *I = cast<Instruction>(U.getUser());
> -    if (DT.dominates(BB, I->getParent())) {
> +    if (DT.properlyDominates(BB, I->getParent())) {
>        U.set(To);
> ----------------
> dberlin wrote:
> > This is definitely not correct.
> > Local to a block, all uses are dominated by their definition, so you
> don't need properlyDominates to replace them legally.
> > It's up to the caller to determine whether this is safe *semantically*.
> >
> > (Even if this change was correct, you've also now made the replacement
> functions completely inconsistent with each other.)
> Since propagateEquality in GVN is the only caller, that's why we changed
> the comments to inform other users that, this function will replace USES
> from the END of the BB.
>
> If the called wants to replace the uses in the BB itself, the other
> overloaded interface replaceDomiantedUsesWith(..... BasicBlockEdge Root)
> can be used.
>


I'd like to hear Piotr's thoughts on all of this.

My view is that if we have to make propagateEquality this complicated to
support assume, we should seriously reconsider how we do propagateEquality
(or split propagateEquality into propagateEquality and propagateAssume, or
*something*).

What is being done here is just making an already hard-to-understand
function *much* harder to understand.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20160112/238a0050/attachment.html>