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

Fri Jul 6 00:02:25 PDT 2018

Hi,

On 07/06/2018 12:17 AM, Mikhail Zolotukhin wrote:
> Hi Mikael,
> 
> I reverted the patch in r336393 until I can fix the issues reported. It should help your case too

Yes it does. Thanks!

/Mikael

, 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
> 