[llvm] r330403 - Reapply "[PR16756] Use SSAUpdaterBulk in JumpThreading." one more time.
Michael Zolotukhin via llvm-commits
llvm-commits at lists.llvm.org
Tue Apr 24 22:33:01 PDT 2018
> On Apr 24, 2018, at 11:11 PM, Hans Wennborg <hans at chromium.org> wrote:
>
> Sorry for the slow reply. I'm afraid I didn't see any build errors at r330434.
Oh well. Thanks for checking!
Michael
>
> 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