[llvm] r330403 - Reapply "[PR16756] Use SSAUpdaterBulk in JumpThreading." one more time.

Hans Wennborg via llvm-commits llvm-commits at lists.llvm.org
Tue Apr 24 13:11:21 PDT 2018


Sorry for the slow reply. I'm afraid I didn't see any build errors at r330434.

On Tue, Apr 24, 2018 at 6:43 AM, Michael Zolotukhin
<mzolotukhin at apple.com> wrote:
> Hi Hans,
>
> It’s just a gentle ping: could you please check if your build succeeded or failed with r330434? Your previous reproducer was of a great value to me and if your build exposes more bugs, I’d really appreciate if you could help me finding reproducers again.
>
> Thanks,
> Michael
>
>> On Apr 20, 2018, at 6:56 PM, Michael Zolotukhin <mzolotukhin at apple.com> wrote:
>>
>> Hi Hans,
>>
>> Does your testing pass with r330434? I fixed the bug from the reproducer you sent to me, but I still see stage3/stage4 miscompares (http://lab.llvm.org:8011/builders/clang-with-thin-lto-ubuntu/builds/9858). Im going to revert the patch (again :-( ), but was wondering if your testing would help me with another reproducer.
>>
>> Thanks,
>> Michael
>>
>>> On Apr 20, 2018, at 3:09 PM, Michael Zolotukhin via llvm-commits <llvm-commits at lists.llvm.org> wrote:
>>>
>>> Thanks, the reproducer works for me! And sorry for the breakage.
>>>
>>> Michael
>>>
>>>> On Apr 20, 2018, at 3:00 PM, Hans Wennborg <hans at chromium.org> wrote:
>>>>
>>>> Uploaded the repro here:
>>>> https://bugs.chromium.org/p/chromium/issues/detail?id=835245#c1
>>>> Please let me know if there are any issues with reproducing.
>>>>
>>>> On Fri, Apr 20, 2018 at 11:51 AM, Ilya Biryukov <ibiryukov at google.com> wrote:
>>>>> +Hans Wennborg , who can provide details for a repro on top of Chromium
>>>>> codebase.
>>>>>
>>>>> On Fri, Apr 20, 2018 at 12:48 PM Michael Zolotukhin <mzolotukhin at apple.com>
>>>>> wrote:
>>>>>>
>>>>>>
>>>>>>
>>>>>> On Apr 20, 2018, at 2:43 PM, Ilya Biryukov <ibiryukov at google.com> wrote:
>>>>>>
>>>>>> This commit seems to crash clang during our integrate while doing PGO
>>>>>> build with the following crash:
>>>>>>    #2 llvm::SSAUpdaterBulk::RewriteAllUses(llvm::DominatorTree*,
>>>>>> llvm::SmallVectorImpl<llvm::PHINode*>*)
>>>>>>    #3 llvm::JumpThreadingPass::ThreadEdge(llvm::BasicBlock*,
>>>>>> llvm::SmallVectorImpl<llvm::BasicBlock*> const&, llvm::BasicBlock*)
>>>>>>    #4 llvm::JumpThreadingPass::ProcessThreadableEdges(llvm::Value*,
>>>>>> llvm::BasicBlock*, llvm::jumpthreading::ConstantPreference,
>>>>>> llvm::Instruction*)
>>>>>>    #5 llvm::JumpThreadingPass::ProcessBlock(llvm::BasicBlock*)
>>>>>>
>>>>>> The crash happens while compiling 'lib/Analysis/CallGraph.cpp'.
>>>>>>
>>>>>> I'm planning to revert it, any objections?
>>>>>>
>>>>>> No objections, but could you please send me a reproducer please? So far
>>>>>> the only testing this change didn’t pass was stage3/stage4 compiler compare
>>>>>> (binaries are different with my change), but this isn’t very helpful when
>>>>>> you don’t know where the bug is.
>>>>>>
>>>>>> Michael
>>>>>>
>>>>>>
>>>>>> On Fri, Apr 20, 2018 at 10:04 AM Michael Zolotukhin via llvm-commits
>>>>>> <llvm-commits at lists.llvm.org> wrote:
>>>>>>>
>>>>>>> Author: mzolotukhin
>>>>>>> Date: Fri Apr 20 01:01:08 2018
>>>>>>> New Revision: 330403
>>>>>>>
>>>>>>> URL: http://llvm.org/viewvc/llvm-project?rev=330403&view=rev
>>>>>>> Log:
>>>>>>> Reapply "[PR16756] Use SSAUpdaterBulk in JumpThreading." one more time.
>>>>>>>
>>>>>>> Hopefully, changing set to vector removes nondeterminism detected by
>>>>>>> some bots, or the new assert will catch something.
>>>>>>>
>>>>>>> This reverts commit r330180.
>>>>>>>
>>>>>>> Added:
>>>>>>>  llvm/trunk/test/Transforms/JumpThreading/removed-use.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=330403&r1=330402&r2=330403&view=diff
>>>>>>>
>>>>>>> ==============================================================================
>>>>>>> --- llvm/trunk/lib/Transforms/Scalar/JumpThreading.cpp (original)
>>>>>>> +++ llvm/trunk/lib/Transforms/Scalar/JumpThreading.cpp Fri Apr 20
>>>>>>> 01:01:08 2018
>>>>>>> @@ -66,6 +66,7 @@
>>>>>>> #include "llvm/Transforms/Utils/BasicBlockUtils.h"
>>>>>>> #include "llvm/Transforms/Utils/Cloning.h"
>>>>>>> #include "llvm/Transforms/Utils/SSAUpdater.h"
>>>>>>> +#include "llvm/Transforms/Utils/SSAUpdaterBulk.h"
>>>>>>> #include "llvm/Transforms/Utils/ValueMapper.h"
>>>>>>> #include <algorithm>
>>>>>>> #include <cassert>
>>>>>>> @@ -1989,15 +1990,20 @@ bool JumpThreadingPass::ThreadEdge(Basic
>>>>>>> // 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
>>>>>>> // PHI insertion, of which we are prepared to do, clean these up now.
>>>>>>> -  SSAUpdater SSAUpdate;
>>>>>>> -  SmallVector<Use*, 16> UsesToRename;
>>>>>>> +  SSAUpdaterBulk SSAUpdate;
>>>>>>> +
>>>>>>> +  unsigned VarNum = 0;
>>>>>>> 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.
>>>>>>> +    // block, 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)
>>>>>>> +        if (UserPN->getIncomingBlock(U) == BB ||
>>>>>>> +            UserPN->getIncomingBlock(U) == PredBB)
>>>>>>>         continue;
>>>>>>>     } else if (User->getParent() == BB)
>>>>>>>       continue;
>>>>>>> @@ -2008,19 +2014,15 @@ bool JumpThreadingPass::ThreadEdge(Basic
>>>>>>>   // If there are no uses outside the block, we're done with this
>>>>>>> instruction.
>>>>>>>   if (UsesToRename.empty())
>>>>>>>     continue;
>>>>>>> +    SSAUpdate.AddVariable(VarNum, I.getName(), I.getType());
>>>>>>>
>>>>>>> -    DEBUG(dbgs() << "JT: Renaming non-local uses of: " << I << "\n");
>>>>>>> -
>>>>>>> -    // We found a use of I outside of BB.  Rename all uses of I that are
>>>>>>> outside
>>>>>>> -    // its block to be uses of the appropriate PHI node etc.  See
>>>>>>> ValuesInBlocks
>>>>>>> -    // with the two values we know.
>>>>>>> -    SSAUpdate.Initialize(I.getType(), I.getName());
>>>>>>> -    SSAUpdate.AddAvailableValue(BB, &I);
>>>>>>> -    SSAUpdate.AddAvailableValue(NewBB, ValueMapping[&I]);
>>>>>>> -
>>>>>>> -    while (!UsesToRename.empty())
>>>>>>> -      SSAUpdate.RewriteUse(*UsesToRename.pop_back_val());
>>>>>>> -    DEBUG(dbgs() << "\n");
>>>>>>> +    // We found a use of I outside of BB - we need to rename all uses of
>>>>>>> I that
>>>>>>> +    // are outside its block to be uses of the appropriate PHI node etc.
>>>>>>> +    SSAUpdate.AddAvailableValue(VarNum, BB, &I);
>>>>>>> +    SSAUpdate.AddAvailableValue(VarNum, NewBB, ValueMapping[&I]);
>>>>>>> +    for (auto *U : UsesToRename)
>>>>>>> +      SSAUpdate.AddUse(VarNum, U);
>>>>>>> +    VarNum++;
>>>>>>> }
>>>>>>>
>>>>>>> // Ok, NewBB is good to go.  Update the terminator of PredBB to jump
>>>>>>> to
>>>>>>> @@ -2037,6 +2039,10 @@ bool JumpThreadingPass::ThreadEdge(Basic
>>>>>>>                    {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
>>>>>>> // over the new instructions and zap any that are constants or dead.
>>>>>>> This
>>>>>>> // frequently happens because of phi translation.
>>>>>>>
>>>>>>> Added: llvm/trunk/test/Transforms/JumpThreading/removed-use.ll
>>>>>>> URL:
>>>>>>> http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Transforms/JumpThreading/removed-use.ll?rev=330403&view=auto
>>>>>>>
>>>>>>> ==============================================================================
>>>>>>> --- llvm/trunk/test/Transforms/JumpThreading/removed-use.ll (added)
>>>>>>> +++ llvm/trunk/test/Transforms/JumpThreading/removed-use.ll Fri Apr 20
>>>>>>> 01:01:08 2018
>>>>>>> @@ -0,0 +1,28 @@
>>>>>>> +; RUN: opt -S < %s -jump-threading | FileCheck %s
>>>>>>> +; CHECK-LABEL: @foo
>>>>>>> +; CHECK: bb6:
>>>>>>> +; CHECK-NEXT: ret void
>>>>>>> +; CHECK: bb3:
>>>>>>> +; CHECK: br label %bb3
>>>>>>> +define void @foo() {
>>>>>>> +entry:
>>>>>>> +  br i1 true, label %bb6, label %bb3
>>>>>>> +
>>>>>>> +bb3:
>>>>>>> +  %x0 = phi i32 [ undef, %entry ], [ %x1, %bb5 ]
>>>>>>> +  %y  = and i64 undef, 1
>>>>>>> +  %p  = icmp ne i64 %y, 0
>>>>>>> +  br i1 %p, label %bb4, label %bb5
>>>>>>> +
>>>>>>> +bb4:
>>>>>>> +  br label %bb5
>>>>>>> +
>>>>>>> +bb5:
>>>>>>> +  %x1 = phi i32 [ %x0, %bb3 ], [ %x0, %bb4 ]
>>>>>>> +  %z  = phi i32 [ 0, %bb3 ], [ 1, %bb4 ]
>>>>>>> +  %q  = icmp eq i32 %z, 0
>>>>>>> +  br i1 %q, label %bb3, label %bb6
>>>>>>> +
>>>>>>> +bb6:
>>>>>>> +  ret void
>>>>>>> +}
>>>>>>>
>>>>>>>
>>>>>>> _______________________________________________
>>>>>>> llvm-commits mailing list
>>>>>>> llvm-commits at lists.llvm.org
>>>>>>> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>>>>>>
>>>>>>
>>>>>>
>>>>>> --
>>>>>> Regards,
>>>>>> Ilya Biryukov
>>>>>>
>>>>>>
>>>>>
>>>>>
>>>>> --
>>>>> Regards,
>>>>> Ilya Biryukov
>>>
>>> _______________________________________________
>>> 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