diff --git a/llvm/include/llvm/Transforms/Scalar/GVN.h b/llvm/include/llvm/Transforms/Scalar/GVN.h index 74a4d6ce00fcc..bc0f108ac8260 100644 --- a/llvm/include/llvm/Transforms/Scalar/GVN.h +++ b/llvm/include/llvm/Transforms/Scalar/GVN.h @@ -28,6 +28,7 @@ #include #include #include +#include #include namespace llvm { @@ -322,11 +323,6 @@ class GVNPass : public PassInfoMixin { }; LeaderMap LeaderTable; - // Block-local map of equivalent values to their leader, does not - // propagate to any successors. Entries added mid-block are applied - // to the remaining instructions in the block. - SmallMapVector ReplaceOperandsWithMap; - // Map the block to reversed postorder traversal number. It is used to // find back edge easily. DenseMap, uint32_t> BlockRPONumber; @@ -402,9 +398,9 @@ class GVNPass : public PassInfoMixin { void verifyRemoved(const Instruction *I) const; bool splitCriticalEdges(); BasicBlock *splitCriticalEdges(BasicBlock *Pred, BasicBlock *Succ); - bool replaceOperandsForInBlockEquality(Instruction *I) const; - bool propagateEquality(Value *LHS, Value *RHS, const BasicBlockEdge &Root, - bool DominatesByEdge); + bool + propagateEquality(Value *LHS, Value *RHS, + const std::variant &Root); bool processFoldableCondBr(BranchInst *BI); void addDeadBlock(BasicBlock *BB); void assignValNumForDeadCode(); diff --git a/llvm/include/llvm/Transforms/Utils/Local.h b/llvm/include/llvm/Transforms/Utils/Local.h index 3f5f4278a2766..9acfd872e574b 100644 --- a/llvm/include/llvm/Transforms/Utils/Local.h +++ b/llvm/include/llvm/Transforms/Utils/Local.h @@ -452,6 +452,11 @@ LLVM_ABI unsigned replaceDominatedUsesWith(Value *From, Value *To, LLVM_ABI unsigned replaceDominatedUsesWith(Value *From, Value *To, DominatorTree &DT, const BasicBlock *BB); +/// Replace each use of 'From' with 'To' if that use is dominated by the +/// given instruction. Returns the number of replacements made. +LLVM_ABI unsigned replaceDominatedUsesWith(Value *From, Value *To, + DominatorTree &DT, + const Instruction *I); /// Replace each use of 'From' with 'To' if that use is dominated by /// the given edge and the callback ShouldReplace returns true. Returns the /// number of replacements made. @@ -464,6 +469,12 @@ LLVM_ABI unsigned replaceDominatedUsesWithIf( LLVM_ABI unsigned replaceDominatedUsesWithIf( Value *From, Value *To, DominatorTree &DT, const BasicBlock *BB, function_ref ShouldReplace); +/// Replace each use of 'From' with 'To' if that use is dominated by +/// the given instruction and the callback ShouldReplace returns true. Returns +/// the number of replacements made. +LLVM_ABI unsigned replaceDominatedUsesWithIf( + Value *From, Value *To, DominatorTree &DT, const Instruction *I, + function_ref ShouldReplace); /// Return true if this call calls a gc leaf function. /// diff --git a/llvm/lib/Transforms/Scalar/GVN.cpp b/llvm/lib/Transforms/Scalar/GVN.cpp index 638952a3aa58f..b9e0fdaebd159 100644 --- a/llvm/lib/Transforms/Scalar/GVN.cpp +++ b/llvm/lib/Transforms/Scalar/GVN.cpp @@ -2084,13 +2084,6 @@ bool GVNPass::processNonLocalLoad(LoadInst *Load) { return Changed; } -static bool hasUsersIn(Value *V, BasicBlock *BB) { - return any_of(V->users(), [BB](User *U) { - auto *I = dyn_cast(U); - return I && I->getParent() == BB; - }); -} - bool GVNPass::processAssumeIntrinsic(AssumeInst *IntrinsicI) { Value *V = IntrinsicI->getArgOperand(0); @@ -2149,85 +2142,7 @@ bool GVNPass::processAssumeIntrinsic(AssumeInst *IntrinsicI) { } Constant *True = ConstantInt::getTrue(V->getContext()); - bool Changed = false; - - for (BasicBlock *Successor : successors(IntrinsicI->getParent())) { - BasicBlockEdge Edge(IntrinsicI->getParent(), Successor); - - // This property is only true in dominated successors, propagateEquality - // will check dominance for us. - Changed |= propagateEquality(V, True, Edge, false); - } - - // We can replace assume value with true, which covers cases like this: - // call void @llvm.assume(i1 %cmp) - // br i1 %cmp, label %bb1, label %bb2 ; will change %cmp to true - ReplaceOperandsWithMap[V] = True; - - // Similarly, after assume(!NotV) we know that NotV == false. - Value *NotV; - if (match(V, m_Not(m_Value(NotV)))) - ReplaceOperandsWithMap[NotV] = ConstantInt::getFalse(V->getContext()); - - // If we find an equality fact, canonicalize all dominated uses in this block - // to one of the two values. We heuristically choice the "oldest" of the - // two where age is determined by value number. (Note that propagateEquality - // above handles the cross block case.) - // - // Key case to cover are: - // 1) - // %cmp = fcmp oeq float 3.000000e+00, %0 ; const on lhs could happen - // call void @llvm.assume(i1 %cmp) - // ret float %0 ; will change it to ret float 3.000000e+00 - // 2) - // %load = load float, float* %addr - // %cmp = fcmp oeq float %load, %0 - // call void @llvm.assume(i1 %cmp) - // ret float %load ; will change it to ret float %0 - if (auto *CmpI = dyn_cast(V)) { - if (CmpI->isEquivalence()) { - Value *CmpLHS = CmpI->getOperand(0); - Value *CmpRHS = CmpI->getOperand(1); - // Heuristically pick the better replacement -- the choice of heuristic - // isn't terribly important here, but the fact we canonicalize on some - // replacement is for exposing other simplifications. - // TODO: pull this out as a helper function and reuse w/ existing - // (slightly different) logic. - if (isa(CmpLHS) && !isa(CmpRHS)) - std::swap(CmpLHS, CmpRHS); - if (!isa(CmpLHS) && isa(CmpRHS)) - std::swap(CmpLHS, CmpRHS); - if ((isa(CmpLHS) && isa(CmpRHS)) || - (isa(CmpLHS) && isa(CmpRHS))) { - // Move the 'oldest' value to the right-hand side, using the value - // number as a proxy for age. - uint32_t LVN = VN.lookupOrAdd(CmpLHS); - uint32_t RVN = VN.lookupOrAdd(CmpRHS); - if (LVN < RVN) - std::swap(CmpLHS, CmpRHS); - } - - // Handle degenerate case where we either haven't pruned a dead path or a - // removed a trivial assume yet. - if (isa(CmpLHS) && isa(CmpRHS)) - return Changed; - - LLVM_DEBUG(dbgs() << "Replacing dominated uses of " - << *CmpLHS << " with " - << *CmpRHS << " in block " - << IntrinsicI->getParent()->getName() << "\n"); - - // Setup the replacement map - this handles uses within the same block. - if (hasUsersIn(CmpLHS, IntrinsicI->getParent())) - ReplaceOperandsWithMap[CmpLHS] = CmpRHS; - - // NOTE: The non-block local cases are handled by the call to - // propagateEquality above; this block is just about handling the block - // local cases. TODO: There's a bunch of logic in propagateEqualiy which - // isn't duplicated for the block local case, can we share it somehow? - } - } - return Changed; + return propagateEquality(V, True, IntrinsicI); } static void patchAndReplaceAllUsesWith(Instruction *I, Value *Repl) { @@ -2526,39 +2441,28 @@ void GVNPass::assignBlockRPONumber(Function &F) { InvalidBlockRPONumbers = false; } -bool GVNPass::replaceOperandsForInBlockEquality(Instruction *Instr) const { - bool Changed = false; - for (unsigned OpNum = 0; OpNum < Instr->getNumOperands(); ++OpNum) { - Use &Operand = Instr->getOperandUse(OpNum); - auto It = ReplaceOperandsWithMap.find(Operand.get()); - if (It != ReplaceOperandsWithMap.end()) { - const DataLayout &DL = Instr->getDataLayout(); - if (!canReplacePointersInUseIfEqual(Operand, It->second, DL)) - continue; - - LLVM_DEBUG(dbgs() << "GVN replacing: " << *Operand << " with " - << *It->second << " in instruction " << *Instr << '\n'); - Instr->setOperand(OpNum, It->second); - Changed = true; - } - } - return Changed; -} - -/// The given values are known to be equal in every block +/// The given values are known to be equal in every use /// dominated by 'Root'. Exploit this, for example by replacing 'LHS' with /// 'RHS' everywhere in the scope. Returns whether a change was made. -/// If DominatesByEdge is false, then it means that we will propagate the RHS -/// value starting from the end of Root.Start. -bool GVNPass::propagateEquality(Value *LHS, Value *RHS, - const BasicBlockEdge &Root, - bool DominatesByEdge) { +/// The Root may either be a basic block edge (for conditions) or an +/// instruction (for assumes). +bool GVNPass::propagateEquality( + Value *LHS, Value *RHS, + const std::variant &Root) { SmallVector, 4> Worklist; Worklist.push_back(std::make_pair(LHS, RHS)); bool Changed = false; - // For speed, compute a conservative fast approximation to - // DT->dominates(Root, Root.getEnd()); - const bool RootDominatesEnd = isOnlyReachableViaThisEdge(Root, DT); + SmallVector DominatedBlocks; + if (const BasicBlockEdge *Edge = std::get_if(&Root)) { + // For speed, compute a conservative fast approximation to + // DT->dominates(Root, Root.getEnd()); + if (isOnlyReachableViaThisEdge(*Edge, DT)) + DominatedBlocks.push_back(Edge->getEnd()); + } else { + Instruction *I = std::get(Root); + for (const auto *Node : DT->getNode(I->getParent())->children()) + DominatedBlocks.push_back(Node->getBlock()); + } while (!Worklist.empty()) { std::pair Item = Worklist.pop_back_val(); @@ -2606,9 +2510,9 @@ bool GVNPass::propagateEquality(Value *LHS, Value *RHS, // using the leader table is about compiling faster, not optimizing better). // The leader table only tracks basic blocks, not edges. Only add to if we // have the simple case where the edge dominates the end. - if (RootDominatesEnd && !isa(RHS) && - canReplacePointersIfEqual(LHS, RHS, DL)) - LeaderTable.insert(LVN, RHS, Root.getEnd()); + if (!isa(RHS) && canReplacePointersIfEqual(LHS, RHS, DL)) + for (const BasicBlock *BB : DominatedBlocks) + LeaderTable.insert(LVN, RHS, BB); // Replace all occurrences of 'LHS' with 'RHS' everywhere in the scope. As // LHS always has at least one use that is not dominated by Root, this will @@ -2618,12 +2522,14 @@ bool GVNPass::propagateEquality(Value *LHS, Value *RHS, auto CanReplacePointersCallBack = [&DL](const Use &U, const Value *To) { return canReplacePointersInUseIfEqual(U, To, DL); }; - unsigned NumReplacements = - DominatesByEdge - ? replaceDominatedUsesWithIf(LHS, RHS, *DT, Root, - CanReplacePointersCallBack) - : replaceDominatedUsesWithIf(LHS, RHS, *DT, Root.getStart(), - CanReplacePointersCallBack); + unsigned NumReplacements; + if (const BasicBlockEdge *Edge = std::get_if(&Root)) + NumReplacements = replaceDominatedUsesWithIf( + LHS, RHS, *DT, *Edge, CanReplacePointersCallBack); + else + NumReplacements = replaceDominatedUsesWithIf( + LHS, RHS, *DT, std::get(Root), + CanReplacePointersCallBack); if (NumReplacements > 0) { Changed = true; @@ -2682,26 +2588,45 @@ bool GVNPass::propagateEquality(Value *LHS, Value *RHS, // If the number we were assigned was brand new then there is no point in // looking for an instruction realizing it: there cannot be one! if (Num < NextNum) { - Value *NotCmp = findLeader(Root.getEnd(), Num); - if (NotCmp && isa(NotCmp)) { - unsigned NumReplacements = - DominatesByEdge - ? replaceDominatedUsesWith(NotCmp, NotVal, *DT, Root) - : replaceDominatedUsesWith(NotCmp, NotVal, *DT, - Root.getStart()); - Changed |= NumReplacements > 0; - NumGVNEqProp += NumReplacements; - // Cached information for anything that uses NotCmp will be invalid. - if (MD) - MD->invalidateCachedPointerInfo(NotCmp); + for (const auto &Entry : LeaderTable.getLeaders(Num)) { + // Only look at leaders that either dominate the start of the edge, + // or are dominated by the end. This check is not necessary for + // correctness, it only discards cases for which the following + // use replacement will not work anyway. + if (const BasicBlockEdge *Edge = std::get_if(&Root)) { + if (!DT->dominates(Entry.BB, Edge->getStart()) && + !DT->dominates(Edge->getEnd(), Entry.BB)) + continue; + } else { + auto *InstBB = std::get(Root)->getParent(); + if (!DT->dominates(Entry.BB, InstBB) && + !DT->dominates(InstBB, Entry.BB)) + continue; + } + + Value *NotCmp = Entry.Val; + if (NotCmp && isa(NotCmp)) { + unsigned NumReplacements; + if (const BasicBlockEdge *Edge = std::get_if(&Root)) + NumReplacements = + replaceDominatedUsesWith(NotCmp, NotVal, *DT, *Edge); + else + NumReplacements = replaceDominatedUsesWith( + NotCmp, NotVal, *DT, std::get(Root)); + Changed |= NumReplacements > 0; + NumGVNEqProp += NumReplacements; + // Cached information for anything that uses NotCmp will be invalid. + if (MD) + MD->invalidateCachedPointerInfo(NotCmp); + } } } // Ensure that any instruction in scope that gets the "A < B" value number // is replaced with false. // The leader table only tracks basic blocks, not edges. Only add to if we // have the simple case where the edge dominates the end. - if (RootDominatesEnd) - LeaderTable.insert(Num, NotVal, Root.getEnd()); + for (const BasicBlock *BB : DominatedBlocks) + LeaderTable.insert(Num, NotVal, BB); continue; } @@ -2789,11 +2714,11 @@ bool GVNPass::processInstruction(Instruction *I) { Value *TrueVal = ConstantInt::getTrue(TrueSucc->getContext()); BasicBlockEdge TrueE(Parent, TrueSucc); - Changed |= propagateEquality(BranchCond, TrueVal, TrueE, true); + Changed |= propagateEquality(BranchCond, TrueVal, TrueE); Value *FalseVal = ConstantInt::getFalse(FalseSucc->getContext()); BasicBlockEdge FalseE(Parent, FalseSucc); - Changed |= propagateEquality(BranchCond, FalseVal, FalseE, true); + Changed |= propagateEquality(BranchCond, FalseVal, FalseE); return Changed; } @@ -2814,7 +2739,7 @@ bool GVNPass::processInstruction(Instruction *I) { // If there is only a single edge, propagate the case value into it. if (SwitchEdges.lookup(Dst) == 1) { BasicBlockEdge E(Parent, Dst); - Changed |= propagateEquality(SwitchCond, Case.getCaseValue(), E, true); + Changed |= propagateEquality(SwitchCond, Case.getCaseValue(), E); } } return Changed; @@ -2942,8 +2867,6 @@ bool GVNPass::processBlock(BasicBlock *BB) { if (DeadBlocks.count(BB)) return false; - // Clearing map before every BB because it can be used only for single BB. - ReplaceOperandsWithMap.clear(); bool ChangedFunction = false; // Since we may not have visited the input blocks of the phis, we can't @@ -2955,11 +2878,8 @@ bool GVNPass::processBlock(BasicBlock *BB) { for (PHINode *PN : PHINodesToRemove) { removeInstruction(PN); } - for (Instruction &Inst : make_early_inc_range(*BB)) { - if (!ReplaceOperandsWithMap.empty()) - ChangedFunction |= replaceOperandsForInBlockEquality(&Inst); + for (Instruction &Inst : make_early_inc_range(*BB)) ChangedFunction |= processInstruction(&Inst); - } return ChangedFunction; } diff --git a/llvm/lib/Transforms/Utils/Local.cpp b/llvm/lib/Transforms/Utils/Local.cpp index b6ca52e2e5682..46f29030ddb05 100644 --- a/llvm/lib/Transforms/Utils/Local.cpp +++ b/llvm/lib/Transforms/Utils/Local.cpp @@ -3246,6 +3246,13 @@ unsigned llvm::replaceDominatedUsesWith(Value *From, Value *To, return ::replaceDominatedUsesWith(From, To, Dominates); } +unsigned llvm::replaceDominatedUsesWith(Value *From, Value *To, + DominatorTree &DT, + const Instruction *I) { + auto Dominates = [&](const Use &U) { return DT.dominates(I, U); }; + return ::replaceDominatedUsesWith(From, To, Dominates); +} + unsigned llvm::replaceDominatedUsesWithIf( Value *From, Value *To, DominatorTree &DT, const BasicBlockEdge &Root, function_ref ShouldReplace) { @@ -3264,6 +3271,15 @@ unsigned llvm::replaceDominatedUsesWithIf( return ::replaceDominatedUsesWith(From, To, DominatesAndShouldReplace); } +unsigned llvm::replaceDominatedUsesWithIf( + Value *From, Value *To, DominatorTree &DT, const Instruction *I, + function_ref ShouldReplace) { + auto DominatesAndShouldReplace = [&](const Use &U) { + return DT.dominates(I, U) && ShouldReplace(U, To); + }; + return ::replaceDominatedUsesWith(From, To, DominatesAndShouldReplace); +} + bool llvm::callsGCLeafFunction(const CallBase *Call, const TargetLibraryInfo &TLI) { // Check if the function is specifically marked as a gc leaf function. diff --git a/llvm/test/Transforms/GVN/condprop.ll b/llvm/test/Transforms/GVN/condprop.ll index eb2a9f1e847c4..57bd2f3487567 100644 --- a/llvm/test/Transforms/GVN/condprop.ll +++ b/llvm/test/Transforms/GVN/condprop.ll @@ -360,7 +360,7 @@ define i1 @test6_phi2(i1 %c, i32 %x, i32 %y) { ; CHECK-NEXT: [[CMP:%.*]] = icmp eq i32 [[X]], [[Y]] ; CHECK-NEXT: br i1 [[CMP]], label [[BB2]], label [[BB3:%.*]] ; CHECK: bb2: -; CHECK-NEXT: [[PHI:%.*]] = phi i1 [ [[CMP_NOT]], [[BB1]] ], [ true, [[ENTRY:%.*]] ] +; CHECK-NEXT: [[PHI:%.*]] = phi i1 [ false, [[BB1]] ], [ true, [[ENTRY:%.*]] ] ; CHECK-NEXT: ret i1 [[PHI]] ; CHECK: bb3: ; CHECK-NEXT: ret i1 false