From: Daniel Berlin Date: Sat, 30 Sep 2017 23:51:54 +0000 (+0000) Subject: NewGVN: Evaluate phi of ops expressions before creating phi node X-Git-Url: https://granicus.if.org/sourcecode?a=commitdiff_plain;h=25bd82ac686ec2c1fec8e863095bc5a63dfa75bc;p=llvm NewGVN: Evaluate phi of ops expressions before creating phi node git-svn-id: https://llvm.org/svn/llvm-project/llvm/trunk@314611 91177308-0d34-0410-b5e6-96231b3b80d8 --- diff --git a/lib/Transforms/Scalar/NewGVN.cpp b/lib/Transforms/Scalar/NewGVN.cpp index f157e168886..ae7df4c0356 100644 --- a/lib/Transforms/Scalar/NewGVN.cpp +++ b/lib/Transforms/Scalar/NewGVN.cpp @@ -587,7 +587,11 @@ private: const Expression *createExpression(Instruction *) const; const Expression *createBinaryExpression(unsigned, Type *, Value *, Value *, Instruction *) const; - PHIExpression *createPHIExpression(Instruction *, bool &HasBackEdge, + // Our canonical form for phi arguments is a pair of incoming value, incoming + // basic block. + typedef std::pair ValPair; + PHIExpression *createPHIExpression(ArrayRef, const Instruction *, + BasicBlock *, bool &HasBackEdge, bool &OriginalOpsConstant) const; const DeadExpression *createDeadExpression() const; const VariableExpression *createVariableExpression(Value *) const; @@ -658,7 +662,10 @@ private: const Expression *performSymbolicLoadEvaluation(Instruction *) const; const Expression *performSymbolicStoreEvaluation(Instruction *) const; const Expression *performSymbolicCallEvaluation(Instruction *) const; - const Expression *performSymbolicPHIEvaluation(Instruction *) const; + void sortPHIOps(MutableArrayRef Ops) const; + const Expression *performSymbolicPHIEvaluation(ArrayRef, + Instruction *I, + BasicBlock *PHIBlock) const; const Expression *performSymbolicAggrValueEvaluation(Instruction *) const; const Expression *performSymbolicCmpEvaluation(Instruction *) const; const Expression *performSymbolicPredicateInfoEvaluation(Instruction *) const; @@ -857,6 +864,16 @@ static bool isCopyOfAPHI(const Value *V) { return CO && isa(CO); } +// Sort PHI Operands into a canonical order. What we use here is an RPO +// order. The BlockInstRange numbers are generated in an RPO walk of the basic +// blocks. +void NewGVN::sortPHIOps(MutableArrayRef Ops) const { + std::sort(Ops.begin(), Ops.end(), [&](const ValPair &P1, const ValPair &P2) { + return BlockInstRange.lookup(P1.second).first < + BlockInstRange.lookup(P2.second).first; + }); +} + // Return true if V is a value that will always be available (IE can // be placed anywhere) in the function. We don't do globals here // because they are often worse to put in place. @@ -864,50 +881,42 @@ static bool alwaysAvailable(Value *V) { return isa(V) || isa(V); } -PHIExpression *NewGVN::createPHIExpression(Instruction *I, bool &HasBackedge, +// Create a PHIExpression from an array of {incoming edge, value} pairs. I is +// the original instruction we are creating a PHIExpression for (but may not be +// a phi node). We require, as an invariant, that all the PHIOperands in the +// same block are sorted the same way. sortPHIOps will sort them into a +// canonical order. +PHIExpression *NewGVN::createPHIExpression(ArrayRef PHIOperands, + const Instruction *I, + BasicBlock *PHIBlock, + bool &HasBackedge, bool &OriginalOpsConstant) const { - BasicBlock *PHIBlock = getBlockForValue(I); - auto *PN = cast(I); - auto *E = - new (ExpressionAllocator) PHIExpression(PN->getNumOperands(), PHIBlock); + unsigned NumOps = PHIOperands.size(); + auto *E = new (ExpressionAllocator) PHIExpression(NumOps, PHIBlock); E->allocateOperands(ArgRecycler, ExpressionAllocator); - E->setType(I->getType()); - E->setOpcode(I->getOpcode()); - - // NewGVN assumes the operands of a PHI node are in a consistent order across - // PHIs. LLVM doesn't seem to always guarantee this. While we need to fix - // this in LLVM at some point we don't want GVN to find wrong congruences. - // Therefore, here we sort uses in predecessor order. - // We're sorting the values by pointer. In theory this might be cause of - // non-determinism, but here we don't rely on the ordering for anything - // significant, e.g. we don't create new instructions based on it so we're - // fine. - SmallVector PHIOperands; - for (const Use &U : PN->operands()) - PHIOperands.push_back(&U); - std::sort(PHIOperands.begin(), PHIOperands.end(), - [&](const Use *U1, const Use *U2) { - return PN->getIncomingBlock(*U1) < PN->getIncomingBlock(*U2); - }); + E->setType(PHIOperands.begin()->first->getType()); + E->setOpcode(Instruction::PHI); // Filter out unreachable phi operands. - auto Filtered = make_filter_range(PHIOperands, [&](const Use *U) { - auto *BB = PN->getIncomingBlock(*U); - if (isCopyOfPHI(*U, PN)) - return false; + auto Filtered = make_filter_range(PHIOperands, [&](const ValPair &P) { + auto *BB = P.second; + if (auto *PHIOp = dyn_cast(I)) + if (isCopyOfPHI(P.first, PHIOp)) + return false; if (!ReachableEdges.count({BB, PHIBlock})) return false; // Things in TOPClass are equivalent to everything. - if (ValueToClass.lookup(*U) == TOPClass) + if (ValueToClass.lookup(P.first) == TOPClass) return false; - OriginalOpsConstant = OriginalOpsConstant && isa(*U); + OriginalOpsConstant = OriginalOpsConstant && isa(P.first); HasBackedge = HasBackedge || isBackedge(BB, PHIBlock); - return lookupOperandLeader(*U) != PN; + return lookupOperandLeader(P.first) != I; }); - std::transform( - Filtered.begin(), Filtered.end(), op_inserter(E), - [&](const Use *U) -> Value * { return lookupOperandLeader(*U); }); + std::transform(Filtered.begin(), Filtered.end(), op_inserter(E), + [&](const ValPair &P) -> Value * { + return lookupOperandLeader(P.first); + }); return E; } @@ -1628,7 +1637,10 @@ bool NewGVN::isCycleFree(const Instruction *I) const { } // Evaluate PHI nodes symbolically and create an expression result. -const Expression *NewGVN::performSymbolicPHIEvaluation(Instruction *I) const { +const Expression * +NewGVN::performSymbolicPHIEvaluation(ArrayRef PHIOps, + Instruction *I, + BasicBlock *PHIBlock) const { // True if one of the incoming phi edges is a backedge. bool HasBackedge = false; // All constant tracks the state of whether all the *original* phi operands @@ -1636,8 +1648,8 @@ const Expression *NewGVN::performSymbolicPHIEvaluation(Instruction *I) const { // change in value of the phi is guaranteed not to later change the value of // the phi. IE it can't be v = phi(undef, v+1) bool OriginalOpsConstant = true; - auto *E = cast( - createPHIExpression(I, HasBackedge, OriginalOpsConstant)); + auto *E = cast(createPHIExpression( + PHIOps, I, PHIBlock, HasBackedge, OriginalOpsConstant)); // We match the semantics of SimplifyPhiNode from InstructionSimplify here. // See if all arguments are the same. // We track if any were undef because they need special handling. @@ -1886,9 +1898,15 @@ NewGVN::performSymbolicEvaluation(Value *V, case Instruction::InsertValue: E = performSymbolicAggrValueEvaluation(I); break; - case Instruction::PHI: - E = performSymbolicPHIEvaluation(I); - break; + case Instruction::PHI: { + SmallVector Ops; + auto *PN = cast(I); + for (unsigned i = 0; i < PN->getNumOperands(); ++i) + Ops.push_back({PN->getIncomingValue(i), PN->getIncomingBlock(i)}); + // Sort to ensure the invariant createPHIExpression requires is met. + sortPHIOps(Ops); + E = performSymbolicPHIEvaluation(Ops, I, getBlockForValue(I)); + } break; case Instruction::Call: E = performSymbolicCallEvaluation(I); break; @@ -2649,7 +2667,7 @@ NewGVN::makePossiblePHIOfOps(Instruction *I, continue; if (!DebugCounter::shouldExecute(PHIOfOpsCounter)) return nullptr; - SmallVector, 4> Ops; + SmallVector Ops; auto *PHIBlock = getBlockForValue(OpPHI); RevisitOnReachabilityChange[PHIBlock].reset(InstrToDFSNum(I)); for (unsigned PredNum = 0; PredNum < OpPHI->getNumOperands(); ++PredNum) { @@ -2685,7 +2703,7 @@ NewGVN::makePossiblePHIOfOps(Instruction *I, (Op != OrigOp || OpIsSafeForPHIOfOps(Op, I, PHIBlock, VisitedOps)); } - // FIXME: For those things that are not safe We could generate + // FIXME: For those things that are not safe we could generate // expressions all the way down, and see if this comes out to a // constant. For anything where that is true, and unsafe, we should // have made a phi-of-ops (or value numbered it equivalent to something) @@ -2708,9 +2726,14 @@ NewGVN::makePossiblePHIOfOps(Instruction *I, DEBUG(dbgs() << "Found phi of ops operand " << *FoundVal << " in " << getBlockName(PredBB) << "\n"); } - - // FIXME: We should evaluate the phi operands first and see if it ends up a - // constant or variable expression. + sortPHIOps(Ops); + auto *E = performSymbolicPHIEvaluation(Ops, I, PHIBlock); + if (isa(E) || isa(E)) { + DEBUG(dbgs() + << "Not creating real PHI of ops because it simplified to existing " + "value or constant\n"); + return E; + } auto *ValuePHI = RealToTemp.lookup(I); bool NewPHI = false; if (!ValuePHI) { @@ -2734,7 +2757,8 @@ NewGVN::makePossiblePHIOfOps(Instruction *I, RevisitOnReachabilityChange[PHIBlock].set(InstrToDFSNum(I)); DEBUG(dbgs() << "Created phi of ops " << *ValuePHI << " for " << *I << "\n"); - return performSymbolicEvaluation(ValuePHI, Visited); + + return E; } return nullptr; } @@ -3095,7 +3119,7 @@ void NewGVN::verifyMemoryCongruency() const { // so we don't process them. if (auto *MemPHI = dyn_cast(Pair.first)) { for (auto &U : MemPHI->incoming_values()) { - if (Instruction *I = dyn_cast(U.get())) { + if (auto *I = dyn_cast(&*U)) { if (!isInstructionTriviallyDead(I)) return true; } diff --git a/test/Transforms/NewGVN/completeness.ll b/test/Transforms/NewGVN/completeness.ll index 4da3413f53d..70ce8dd0159 100644 --- a/test/Transforms/NewGVN/completeness.ll +++ b/test/Transforms/NewGVN/completeness.ll @@ -8,7 +8,7 @@ define i32 @test1(i32, i8**) { ; CHECK-NEXT: br i1 [[TMP3]], label [[TMP4:%.*]], label [[TMP5:%.*]] ; CHECK: br label [[TMP6:%.*]] ; CHECK: br label [[TMP6]] -; CHECK: [[PHIOFOPS:%.*]] = phi i32 [ 75, [[TMP4]] ], [ 105, [[TMP5]] ] +; CHECK: [[PHIOFOPS:%.*]] = phi i32 [ 105, [[TMP5]] ], [ 75, [[TMP4]] ] ; CHECK-NEXT: [[DOT0:%.*]] = phi i32 [ 5, [[TMP4]] ], [ 7, [[TMP5]] ] ; CHECK-NEXT: ret i32 [[PHIOFOPS]] ; @@ -33,8 +33,8 @@ define i32 @test1b(i32, i8**) { ; CHECK-NEXT: br i1 [[TMP3]], label [[TMP4:%.*]], label [[TMP5:%.*]] ; CHECK: br label [[TMP6:%.*]] ; CHECK: br label [[TMP6]] -; CHECK: [[PHIOFOPS1:%.*]] = phi i32 [ 75, [[TMP4]] ], [ 105, [[TMP5]] ] -; CHECK-NEXT: [[PHIOFOPS:%.*]] = phi i32 [ 1125, [[TMP4]] ], [ 1575, [[TMP5]] ] +; CHECK: [[PHIOFOPS1:%.*]] = phi i32 [ 105, [[TMP5]] ], [ 75, [[TMP4]] ] +; CHECK-NEXT: [[PHIOFOPS:%.*]] = phi i32 [ 1575, [[TMP5]] ], [ 1125, [[TMP4]] ] ; CHECK-NEXT: [[DOT0:%.*]] = phi i32 [ 5, [[TMP4]] ], [ 7, [[TMP5]] ] ; CHECK-NEXT: ret i32 [[PHIOFOPS]] ; @@ -215,7 +215,7 @@ define i64 @test5(i64 %arg) { ; CHECK: bb14: ; CHECK-NEXT: br label [[BB15:%.*]] ; CHECK: bb15: -; CHECK-NEXT: [[PHIOFOPS:%.*]] = phi i64 [ [[TMP25:%.*]], [[BB15]] ], [ [[TMP12]], [[BB14]] ] +; CHECK-NEXT: [[PHIOFOPS:%.*]] = phi i64 [ [[TMP12]], [[BB14]] ], [ [[TMP25:%.*]], [[BB15]] ] ; CHECK-NEXT: [[TMP16:%.*]] = phi i64 [ [[TMP24:%.*]], [[BB15]] ], [ [[TMP11]], [[BB14]] ] ; CHECK-NEXT: [[TMP17:%.*]] = phi i64 [ [[TMP22:%.*]], [[BB15]] ], [ [[TMP10]], [[BB14]] ] ; CHECK-NEXT: [[TMP18:%.*]] = phi i64 [ [[TMP20:%.*]], [[BB15]] ], [ 0, [[BB14]] ] diff --git a/test/Transforms/NewGVN/pr33461.ll b/test/Transforms/NewGVN/pr33461.ll index 5ed66ab7918..85e8b68693b 100644 --- a/test/Transforms/NewGVN/pr33461.ll +++ b/test/Transforms/NewGVN/pr33461.ll @@ -8,7 +8,7 @@ define void @patatino() { ; CHECK-NEXT: entry: ; CHECK-NEXT: br i1 false, label [[FOR_COND1:%.*]], label [[FOR_INC:%.*]] ; CHECK: for.cond1: -; CHECK-NEXT: [[PHIOFOPS:%.*]] = phi i16 [ [[INC:%.*]], [[FOR_INC]] ], [ undef, [[ENTRY:%.*]] ] +; CHECK-NEXT: [[PHIOFOPS:%.*]] = phi i16 [ undef, [[ENTRY:%.*]] ], [ [[INC:%.*]], [[FOR_INC]] ] ; CHECK-NEXT: store i16 [[PHIOFOPS]], i16* @b, align 2 ; CHECK-NEXT: br label [[FOR_INC]] ; CHECK: for.inc: diff --git a/test/Transforms/NewGVN/storeoverstore.ll b/test/Transforms/NewGVN/storeoverstore.ll index 2117d0ee060..385f1875778 100644 --- a/test/Transforms/NewGVN/storeoverstore.ll +++ b/test/Transforms/NewGVN/storeoverstore.ll @@ -13,11 +13,11 @@ define i32 @foo(i32*, i32) { ; CHECK-NEXT: [[TMP3:%.*]] = icmp ne i32 [[TMP1:%.*]], 0 ; CHECK-NEXT: br i1 [[TMP3]], label [[TMP4:%.*]], label [[TMP5:%.*]] ; CHECK: br label [[TMP5]] -; CHECK: [[TMP6:%.*]] = phi i32 [ 15, [[TMP4]] ], [ 10, [[TMP2:%.*]] ] +; CHECK: [[PHIOFOPS:%.*]] = phi i32 [ 10, [[TMP2:%.*]] ], [ 15, [[TMP4]] ] ; CHECK-NEXT: [[DOT0:%.*]] = phi i32 [ 10, [[TMP4]] ], [ 5, [[TMP2]] ] -; CHECK-NEXT: br i1 [[TMP3]], label [[TMP7:%.*]], label [[TMP8:%.*]] -; CHECK: br label [[TMP8]] -; CHECK: [[DOT1:%.*]] = phi i32 [ [[TMP6]], [[TMP7]] ], [ [[DOT0]], [[TMP5]] ] +; CHECK-NEXT: br i1 [[TMP3]], label [[TMP6:%.*]], label [[TMP7:%.*]] +; CHECK: br label [[TMP7]] +; CHECK: [[DOT1:%.*]] = phi i32 [ [[PHIOFOPS]], [[TMP6]] ], [ [[DOT0]], [[TMP5]] ] ; CHECK-NEXT: ret i32 [[DOT1]] ; store i32 5, i32* %0, align 4 @@ -54,11 +54,11 @@ define i32 @foo2(i32*, i32) { ; CHECK-NEXT: br i1 [[TMP3]], label [[TMP4:%.*]], label [[TMP5:%.*]] ; CHECK: br label [[TMP6:%.*]] ; CHECK: br label [[TMP6]] -; CHECK: [[TMP7:%.*]] = phi i32 [ 15, [[TMP4]] ], [ 10, [[TMP5]] ] +; CHECK: [[PHIOFOPS:%.*]] = phi i32 [ 10, [[TMP5]] ], [ 15, [[TMP4]] ] ; CHECK-NEXT: [[DOT0:%.*]] = phi i32 [ 10, [[TMP4]] ], [ 5, [[TMP5]] ] -; CHECK-NEXT: br i1 [[TMP3]], label [[TMP8:%.*]], label [[TMP9:%.*]] -; CHECK: br label [[TMP9]] -; CHECK: [[DOT1:%.*]] = phi i32 [ [[TMP7]], [[TMP8]] ], [ [[DOT0]], [[TMP6]] ] +; CHECK-NEXT: br i1 [[TMP3]], label [[TMP7:%.*]], label [[TMP8:%.*]] +; CHECK: br label [[TMP8]] +; CHECK: [[DOT1:%.*]] = phi i32 [ [[PHIOFOPS]], [[TMP7]] ], [ [[DOT0]], [[TMP6]] ] ; CHECK-NEXT: ret i32 [[DOT1]] ; store i32 5, i32* %0, align 4