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

[Daniel Berlin via llvm-commits]

Also note, it's possible to have a case where root.start dominates but it
is not safe to propagate due to multiple edges from that block to to
root.getEnd.
The whole reason for having the edge is to avoid this case, AFAIK.

So i'm pretty sure using root.getStart() is just going to cause incorrect
transforms.

Note that all of propagateEquality is really somewhat of a hack to avoid
using proper control dependence, and at some point, we'll likely want to
just rip it out and use post-dominators to do this *right*.

I don't know whether the optimization capability we lose here by avoiding
assume in some cases makes us want to do that more (I leave that question
to data and chandler)




On Tue, Jan 12, 2016 at 11:07 AM, Daniel Berlin <dberlin at dberlin.org> wrote:

> dberlin added inline comments.
>
> ================
> Comment at: include/llvm/Transforms/Utils/Local.h:320
> @@ -320,2 +319,3 @@
> +/// the end of the given BasicBlock. Returns the number of replacements
> made.
>  unsigned replaceDominatedUsesWith(Value *From, Value *To, DominatorTree
> &DT,
>                                    const BasicBlock *BB);
> ----------------
> I'm not sure this wording change makes sense.
>
> It doesn't care whether it's the end of the block that dominates or not.
> And by definition, uses in the same-block as their definitions must be
> dominated by those defs.
>
>
>
> ================
> 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.
> ----------------
> 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).
>
> ================
> 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());
>
> ----------------
> 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.
>
>
> ================
> Comment at: lib/Transforms/Scalar/GVN.cpp:2277
> @@ -2273,3 +2276,3 @@
>                    : replaceDominatedUsesWith(NotCmp, NotVal, *DT,
> -                                             Root.getEnd());
> +                                             Root.getStart());
>            Changed |= NumReplacements > 0;
> ----------------
> Same comment as above :)
>
> ================
> 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);
> ----------------
> 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.)
>
>
> http://reviews.llvm.org/D16100
>
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20160112/7fd88cd8/attachment.html>