[llvm] r335675 - [JumpThreading] Don't try to rewrite a use if it's already valid.
Mikael Holmén via llvm-commits
llvm-commits at lists.llvm.org
Fri Jun 29 02:36:57 PDT 2018
Aaaand here comes the reproducer as well.
/Mikael
On 06/29/2018 11:36 AM, Mikael Holmén wrote:
> 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
>>
-------------- next part --------------
define void @f() {
entry:
br i1 false, label %label, label %if.end
label: ; preds = %if.end, %entry
br label %if.end
if.end: ; preds = %label, %entry
%check = phi i16 [ 1, %label ], [ 0, %entry ]
%cmp = icmp eq i16 %check, 0
br i1 %cmp, label %label, label %if.end2
if.end2: ; preds = %if.end
call void @g(i16 %check)
ret void
}
declare void @g(i16 %n)
More information about the llvm-commits
mailing list