[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 08:56:04 PDT 2018
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