[llvm] r335675 - [JumpThreading] Don't try to rewrite a use if it's already valid.

[Mikael Holmén via llvm-commits]

Hi Michael,

I get a miscompile from -jump-threading with this patch:

Reproduce with:

  opt -jump-threading -S -o - bbi-15872.ll

%check should be 0 first time we get to %if.end, then we should jump to 
%label, then %if.end, set %check to 1, and jump to %if.end2 where we 
should pass %check (which is now 1) to @g.

But after jump-threading the whole thing is replaced with just

   call void @g(i16 0)

If I revert this commit I instead get the expected

   call void @g(i16 1)

Regards,
Mikael

On 06/27/2018 12:19 AM, Michael Zolotukhin via llvm-commits wrote:
> Author: mzolotukhin
> Date: Tue Jun 26 15:19:48 2018
> New Revision: 335675
> 
> URL: http://llvm.org/viewvc/llvm-project?rev=335675&view=rev
> Log:
> [JumpThreading] Don't try to rewrite a use if it's already valid.
> 
> Summary:
> When recording uses we need to rewrite after cloning a loop we need to
> check if the use is not dominated by the original def. The initial
> assumption was that the cloned basic block will introduce a new path and
> thus the original def will only dominate the use if they are in the same
> BB, but as the reproducer from PR37745 shows it's not always the case.
> 
> This fixes PR37745.
> 
> Reviewers: haicheng, Ka-Ka
> 
> Subscribers: hiraditya, llvm-commits
> 
> Differential Revision: https://reviews.llvm.org/D48111
> 
> Added:
>      llvm/trunk/test/Transforms/JumpThreading/PR37745.ll
> Modified:
>      llvm/trunk/lib/Transforms/Scalar/JumpThreading.cpp
> 
> Modified: llvm/trunk/lib/Transforms/Scalar/JumpThreading.cpp
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/Scalar/JumpThreading.cpp?rev=335675&r1=335674&r2=335675&view=diff
> ==============================================================================
> --- llvm/trunk/lib/Transforms/Scalar/JumpThreading.cpp (original)
> +++ llvm/trunk/lib/Transforms/Scalar/JumpThreading.cpp Tue Jun 26 15:19:48 2018
> @@ -2009,6 +2009,14 @@ bool JumpThreadingPass::ThreadEdge(Basic
>         PredTerm->setSuccessor(i, NewBB);
>       }
>   
> +  // Enqueue required DT updates.
> +  DDT->applyUpdates({{DominatorTree::Insert, NewBB, SuccBB},
> +                     {DominatorTree::Insert, PredBB, NewBB},
> +                     {DominatorTree::Delete, PredBB, BB}});
> +
> +  // Apply all updates we queued with DDT and get the updated Dominator Tree.
> +  DominatorTree *DT = &DDT->flush();
> +
>     // If there were values defined in BB that are used outside the block, then we
>     // now have to update all uses of the value to use either the original value,
>     // the cloned value, or some PHI derived value.  This can require arbitrary
> @@ -2018,16 +2026,16 @@ bool JumpThreadingPass::ThreadEdge(Basic
>     for (Instruction &I : *BB) {
>       SmallVector<Use*, 16> UsesToRename;
>   
> -    // Scan all uses of this instruction to see if it is used outside of its
> -    // block, and if so, record them in UsesToRename. Also, skip phi operands
> -    // from PredBB - we'll remove them anyway.
> +    // Scan all uses of this instruction to see if their uses are no longer
> +    // dominated by the previous def and if so, record them in UsesToRename.
> +    // Also, skip phi operands from PredBB - we'll remove them anyway.
>       for (Use &U : I.uses()) {
>         Instruction *User = cast<Instruction>(U.getUser());
>         if (PHINode *UserPN = dyn_cast<PHINode>(User)) {
>           if (UserPN->getIncomingBlock(U) == BB ||
>               UserPN->getIncomingBlock(U) == PredBB)
>             continue;
> -      } else if (User->getParent() == BB)
> +      } else if (DT->dominates(&I, U))
>           continue;
>   
>         UsesToRename.push_back(&U);
> @@ -2046,12 +2054,6 @@ bool JumpThreadingPass::ThreadEdge(Basic
>         SSAUpdate.AddUse(VarNum, U);
>     }
>   
> -  DDT->applyUpdates({{DominatorTree::Insert, NewBB, SuccBB},
> -                     {DominatorTree::Insert, PredBB, NewBB},
> -                     {DominatorTree::Delete, PredBB, BB}});
> -
> -  // Apply all updates we queued with DDT and get the updated Dominator Tree.
> -  DominatorTree *DT = &DDT->flush();
>     SSAUpdate.RewriteAllUses(DT);
>   
>     // At this point, the IR is fully up to date and consistent.  Do a quick scan
> 
> Added: llvm/trunk/test/Transforms/JumpThreading/PR37745.ll
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Transforms/JumpThreading/PR37745.ll?rev=335675&view=auto
> ==============================================================================
> --- llvm/trunk/test/Transforms/JumpThreading/PR37745.ll (added)
> +++ llvm/trunk/test/Transforms/JumpThreading/PR37745.ll Tue Jun 26 15:19:48 2018
> @@ -0,0 +1,19 @@
> +; RUN: opt -jump-threading -verify-each -S -mtriple=x86_64-- -o - %s
> +
> +define void @foo() {
> +entry:
> +  br i1 false, label %A, label %B
> +
> +A:
> +  %x = phi i32 [ undef, %entry ], [ %z, %B ]
> +  br label %B
> +
> +B:
> +  %y = phi i32 [ undef, %entry ], [ %x, %A ]
> +  %z = add i32 %y, 1
> +  %cmp = icmp ne i32 %z, 0
> +  br i1 %cmp, label %exit, label %A
> +
> +exit:
> +  ret void
> +}
> 
> 
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>