From 5052dcf3b2f5bbad7e51bd4cacc1e2e47b842f04 Mon Sep 17 00:00:00 2001 From: Bardia Mahjour Date: Thu, 7 Jan 2021 10:31:11 -0500 Subject: [PATCH] [DDG] Fix duplicate edge removal during pi-block formation When creating pi-blocks we try to avoid creating duplicate edges between outside nodes and the pi-block when an edge is of the same kind and direction as another one that has already been created. We do this by keeping track of the edges in an enumerated array called EdgeAlreadyCreated. The problem is that this array is declared local to the loop that iterates over the nodes in the pi-block, so the information gets lost every time a new inside-node is iterated over. The fix is to move the declaration to the outer loop. Reviewed By: Meinersbur Differential Revision: https://reviews.llvm.org/D94094 --- lib/Analysis/DependenceGraphBuilder.cpp | 123 ++++++++++--------- unittests/Analysis/DDGTest.cpp | 154 ++++++++++++++++++++++++ 2 files changed, 215 insertions(+), 62 deletions(-) diff --git a/lib/Analysis/DependenceGraphBuilder.cpp b/lib/Analysis/DependenceGraphBuilder.cpp index fbf0f665159..04a34472659 100644 --- a/lib/Analysis/DependenceGraphBuilder.cpp +++ b/lib/Analysis/DependenceGraphBuilder.cpp @@ -140,75 +140,74 @@ template void AbstractDependenceGraphBuilder::createPiBlocks() { if (*N == PiNode || NodesInSCC.count(N)) continue; - for (NodeType *SCCNode : NL) { + enum Direction { + Incoming, // Incoming edges to the SCC + Outgoing, // Edges going ot of the SCC + DirectionCount // To make the enum usable as an array index. + }; - enum Direction { - Incoming, // Incoming edges to the SCC - Outgoing, // Edges going ot of the SCC - DirectionCount // To make the enum usable as an array index. - }; + // Use these flags to help us avoid creating redundant edges. If there + // are more than one edges from an outside node to inside nodes, we only + // keep one edge from that node to the pi-block node. Similarly, if + // there are more than one edges from inside nodes to an outside node, + // we only keep one edge from the pi-block node to the outside node. + // There is a flag defined for each direction (incoming vs outgoing) and + // for each type of edge supported, using a two-dimensional boolean + // array. + using EdgeKind = typename EdgeType::EdgeKind; + EnumeratedArray EdgeAlreadyCreated[DirectionCount]{false, + false}; - // Use these flags to help us avoid creating redundant edges. If there - // are more than one edges from an outside node to inside nodes, we only - // keep one edge from that node to the pi-block node. Similarly, if - // there are more than one edges from inside nodes to an outside node, - // we only keep one edge from the pi-block node to the outside node. - // There is a flag defined for each direction (incoming vs outgoing) and - // for each type of edge supported, using a two-dimensional boolean - // array. - using EdgeKind = typename EdgeType::EdgeKind; - EnumeratedArray EdgeAlreadyCreated[DirectionCount]{ - false, false}; + auto createEdgeOfKind = [this](NodeType &Src, NodeType &Dst, + const EdgeKind K) { + switch (K) { + case EdgeKind::RegisterDefUse: + createDefUseEdge(Src, Dst); + break; + case EdgeKind::MemoryDependence: + createMemoryEdge(Src, Dst); + break; + case EdgeKind::Rooted: + createRootedEdge(Src, Dst); + break; + default: + llvm_unreachable("Unsupported type of edge."); + } + }; - auto createEdgeOfKind = [this](NodeType &Src, NodeType &Dst, - const EdgeKind K) { - switch (K) { - case EdgeKind::RegisterDefUse: - createDefUseEdge(Src, Dst); - break; - case EdgeKind::MemoryDependence: - createMemoryEdge(Src, Dst); - break; - case EdgeKind::Rooted: - createRootedEdge(Src, Dst); - break; - default: - llvm_unreachable("Unsupported type of edge."); - } - }; + auto reconnectEdges = [&](NodeType *Src, NodeType *Dst, NodeType *New, + const Direction Dir) { + if (!Src->hasEdgeTo(*Dst)) + return; + LLVM_DEBUG( + dbgs() << "reconnecting(" + << (Dir == Direction::Incoming ? "incoming)" : "outgoing)") + << ":\nSrc:" << *Src << "\nDst:" << *Dst << "\nNew:" << *New + << "\n"); + assert((Dir == Direction::Incoming || Dir == Direction::Outgoing) && + "Invalid direction."); - auto reconnectEdges = [&](NodeType *Src, NodeType *Dst, NodeType *New, - const Direction Dir) { - if (!Src->hasEdgeTo(*Dst)) - return; - LLVM_DEBUG(dbgs() - << "reconnecting(" - << (Dir == Direction::Incoming ? "incoming)" : "outgoing)") - << ":\nSrc:" << *Src << "\nDst:" << *Dst - << "\nNew:" << *New << "\n"); - assert((Dir == Direction::Incoming || Dir == Direction::Outgoing) && - "Invalid direction."); - - SmallVector EL; - Src->findEdgesTo(*Dst, EL); - for (EdgeType *OldEdge : EL) { - EdgeKind Kind = OldEdge->getKind(); - if (!EdgeAlreadyCreated[Dir][Kind]) { - if (Dir == Direction::Incoming) { - createEdgeOfKind(*Src, *New, Kind); - LLVM_DEBUG(dbgs() << "created edge from Src to New.\n"); - } else if (Dir == Direction::Outgoing) { - createEdgeOfKind(*New, *Dst, Kind); - LLVM_DEBUG(dbgs() << "created edge from New to Dst.\n"); - } - EdgeAlreadyCreated[Dir][Kind] = true; + SmallVector EL; + Src->findEdgesTo(*Dst, EL); + for (EdgeType *OldEdge : EL) { + EdgeKind Kind = OldEdge->getKind(); + if (!EdgeAlreadyCreated[Dir][Kind]) { + if (Dir == Direction::Incoming) { + createEdgeOfKind(*Src, *New, Kind); + LLVM_DEBUG(dbgs() << "created edge from Src to New.\n"); + } else if (Dir == Direction::Outgoing) { + createEdgeOfKind(*New, *Dst, Kind); + LLVM_DEBUG(dbgs() << "created edge from New to Dst.\n"); } - Src->removeEdge(*OldEdge); - destroyEdge(*OldEdge); - LLVM_DEBUG(dbgs() << "removed old edge between Src and Dst.\n\n"); + EdgeAlreadyCreated[Dir][Kind] = true; } - }; + Src->removeEdge(*OldEdge); + destroyEdge(*OldEdge); + LLVM_DEBUG(dbgs() << "removed old edge between Src and Dst.\n\n"); + } + }; + for (NodeType *SCCNode : NL) { // Process incoming edges incident to the pi-block node. reconnectEdges(N, SCCNode, &PiNode, Direction::Incoming); diff --git a/unittests/Analysis/DDGTest.cpp b/unittests/Analysis/DDGTest.cpp index 9919d055d01..b1d35596c1e 100644 --- a/unittests/Analysis/DDGTest.cpp +++ b/unittests/Analysis/DDGTest.cpp @@ -128,3 +128,157 @@ TEST(DDGTest, getDependencies) { SE.getOne(DL.back()->getDistance(1)->getType())); }); } + +/// Test to make sure that when pi-blocks are formed, multiple edges of the same +/// kind and direction are collapsed into a single edge. +/// In the test below, %loadASubI belongs to an outside node, which has input +/// dependency with multiple load instructions in the pi-block containing +/// %loadBSubI. We expect a single memory dependence edge from the outside node +/// to this pi-block. The pi-block also contains %add and %add7 both of which +/// feed a phi in an outside node. We expect a single def-use edge from the +/// pi-block to the node containing that phi. +TEST(DDGTest, avoidDuplicateEdgesToFromPiBlocks) { + const char *ModuleStr = + "target datalayout = \"e-m:e-i64:64-n32:64-v256:256:256-v512:512:512\"\n" + "\n" + "define void @foo(float* noalias %A, float* noalias %B, float* noalias " + "%C, float* noalias %D, i32 signext %n) {\n" + "entry:\n" + " %cmp1 = icmp sgt i32 %n, 0\n" + " br i1 %cmp1, label %for.body.preheader, label %for.end\n" + "\n" + "for.body.preheader: ; preds = %entry\n" + " %wide.trip.count = zext i32 %n to i64\n" + " br label %for.body\n" + "\n" + "for.body: ; preds = " + "%for.body.preheader, %if.end\n" + " %indvars.iv = phi i64 [ 0, %for.body.preheader ], [ %indvars.iv.next, " + "%if.end ]\n" + " %arrayidx = getelementptr inbounds float, float* %A, i64 %indvars.iv\n" + " %loadASubI = load float, float* %arrayidx, align 4\n" + " %arrayidx2 = getelementptr inbounds float, float* %B, i64 " + "%indvars.iv\n" + " %loadBSubI = load float, float* %arrayidx2, align 4\n" + " %add = fadd fast float %loadASubI, %loadBSubI\n" + " %arrayidx4 = getelementptr inbounds float, float* %A, i64 " + "%indvars.iv\n" + " store float %add, float* %arrayidx4, align 4\n" + " %arrayidx6 = getelementptr inbounds float, float* %A, i64 " + "%indvars.iv\n" + " %0 = load float, float* %arrayidx6, align 4\n" + " %add7 = fadd fast float %0, 1.000000e+00\n" + " %indvars.iv.next = add nuw nsw i64 %indvars.iv, 1\n" + " %arrayidx10 = getelementptr inbounds float, float* %B, i64 " + "%indvars.iv.next\n" + " store float %add7, float* %arrayidx10, align 4\n" + " %arrayidx12 = getelementptr inbounds float, float* %A, i64 " + "%indvars.iv\n" + " %1 = load float, float* %arrayidx12, align 4\n" + " %cmp13 = fcmp fast ogt float %1, 1.000000e+02\n" + " br i1 %cmp13, label %if.then, label %if.else\n" + "\n" + "if.then: ; preds = %for.body\n" + " br label %if.end\n" + "\n" + "if.else: ; preds = %for.body\n" + " br label %if.end\n" + "\n" + "if.end: ; preds = %if.else, " + "%if.then\n" + " %ff.0 = phi float [ %add, %if.then ], [ %add7, %if.else ]\n" + " store float %ff.0, float* %C, align 4\n" + " %exitcond = icmp ne i64 %indvars.iv.next, %wide.trip.count\n" + " br i1 %exitcond, label %for.body, label %for.end.loopexit\n" + "\n" + "for.end.loopexit: ; preds = %if.end\n" + " br label %for.end\n" + "\n" + "for.end: ; preds = " + "%for.end.loopexit, %entry\n" + " ret void\n" + "}\n"; + + LLVMContext Context; + std::unique_ptr M = makeLLVMModule(Context, ModuleStr); + + runTest( + *M, "foo", + [&](Function &F, LoopInfo &LI, DependenceInfo &DI, ScalarEvolution &SE) { + Loop *L = *LI.begin(); + assert(L && "expected the loop to be identified."); + + DataDependenceGraph DDG(*L, LI, DI); + + const DDGNode *LoadASubI = nullptr; + for (DDGNode *N : DDG) { + if (!isa(N)) + continue; + SmallVector IList; + N->collectInstructions([](const Instruction *I) { return true; }, + IList); + if (llvm::any_of(IList, [](Instruction *I) { + return I->getName() == "loadASubI"; + })) { + LoadASubI = N; + break; + } + } + assert(LoadASubI && "Did not find load of A[i]"); + + const PiBlockDDGNode *PiBlockWithBSubI = nullptr; + for (DDGNode *N : DDG) { + if (!isa(N)) + continue; + for (DDGNode *M : cast(N)->getNodes()) { + SmallVector IList; + M->collectInstructions([](const Instruction *I) { return true; }, + IList); + if (llvm::any_of(IList, [](Instruction *I) { + return I->getName() == "loadBSubI"; + })) { + PiBlockWithBSubI = static_cast(N); + break; + } + } + if (PiBlockWithBSubI) + break; + } + assert(PiBlockWithBSubI && + "Did not find pi-block containing load of B[i]"); + + const DDGNode *FFPhi = nullptr; + for (DDGNode *N : DDG) { + if (!isa(N)) + continue; + SmallVector IList; + N->collectInstructions([](const Instruction *I) { return true; }, + IList); + if (llvm::any_of(IList, [](Instruction *I) { + return I->getName() == "ff.0"; + })) { + FFPhi = N; + break; + } + } + assert(FFPhi && "Did not find ff.0 phi instruction"); + + // Expect a single memory edge from '%0 = A[i]' to the pi-block. This + // means the duplicate incoming memory edges are removed during pi-block + // formation. + SmallVector EL; + LoadASubI->findEdgesTo(*PiBlockWithBSubI, EL); + unsigned NumMemoryEdges = llvm::count_if( + EL, [](DDGEdge *Edge) { return Edge->isMemoryDependence(); }); + EXPECT_EQ(NumMemoryEdges, 1ull); + + /// Expect a single def-use edge from the pi-block to '%ff.0 = phi...`. + /// This means the duplicate outgoing def-use edges are removed during + /// pi-block formation. + EL.clear(); + PiBlockWithBSubI->findEdgesTo(*FFPhi, EL); + NumMemoryEdges = + llvm::count_if(EL, [](DDGEdge *Edge) { return Edge->isDefUse(); }); + EXPECT_EQ(NumMemoryEdges, 1ull); + }); +}