[llvm] r335675 - [JumpThreading] Don't try to rewrite a use if it's already valid.
Mikhail Zolotukhin via llvm-commits
llvm-commits at lists.llvm.org
Thu Jul 5 15:17:11 PDT 2018
Hi Mikael,
I reverted the patch in r336393 until I can fix the issues reported. It should help your case too, but please let me know if it doesn’t!
Thanks,
Michael
> On Jul 3, 2018, at 1:12 PM, Michael Zolotukhin via llvm-commits <llvm-commits at lists.llvm.org> wrote:
>
> Hi Mikael,
>
> Sorry, got pulled away by other stuff, I’ll try to investigate this asap.
>
> Michael
>
>> On Jul 3, 2018, at 12:04 AM, Mikael Holmén <mikael.holmen at ericsson.com> wrote:
>>
>> Hi,
>>
>> On 06/29/2018 05:28 PM, Michael Zolotukhin wrote:
>>> Thanks for the reproducer, I’ll take a look!
>>
>> Sounds good! Did you get anywhere with this?
>>
>> Regards,
>> Mikael
>>
>>> Michael
>>>> On Jun 29, 2018, at 2:36 AM, Mikael Holmén <mikael.holmen at ericsson.com> wrote:
>>>>
>>>> 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
>>>>>>
>>>> <bbi-15872.ll>
>>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
More information about the llvm-commits
mailing list