[llvm] r330403 - Reapply "[PR16756] Use SSAUpdaterBulk in JumpThreading." one more time.
Michael Zolotukhin via llvm-commits
llvm-commits at lists.llvm.org
Fri Apr 20 04:09:41 PDT 2018
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
More information about the llvm-commits
mailing list