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

Fri Apr 20 04:00:21 PDT 2018

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