[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

Author: bmahjour

llvm/lib/Analysis/DependenceGraphBuilder.cpp

@@ -140,75 +140,74 @@ template <class G> void AbstractDependenceGraphBuilder<G>::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.
-        };
-
-        // 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<bool, EdgeKind> 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 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<EdgeType *, 10> 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;
+      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<bool, EdgeKind> 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 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<EdgeType *, 10> 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);
 

llvm/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<Module> 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<SimpleDDGNode>(N))
+            continue;
+          SmallVector<Instruction *, 8> 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<PiBlockDDGNode>(N))
+            continue;
+          for (DDGNode *M : cast<PiBlockDDGNode>(N)->getNodes()) {
+            SmallVector<Instruction *, 8> IList;
+            M->collectInstructions([](const Instruction *I) { return true; },
+                                   IList);
+            if (llvm::any_of(IList, [](Instruction *I) {
+                  return I->getName() == "loadBSubI";
+                })) {
+              PiBlockWithBSubI = static_cast<PiBlockDDGNode *>(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<SimpleDDGNode>(N))
+            continue;
+          SmallVector<Instruction *, 8> 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<DDGEdge *, 4> 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);
+      });
+}