[DDG] Fix duplicate edge removal during pi-block formation

[bmahjour]

Summary

I stumbled upon this issue while working on the graph printer. 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.

An example is provided in the unittest added in this revision.

[Meinersbur]

LGTM.

Consider clang-format before pushing to main.

[bmahjour]

clang-format

llvm/lib/Analysis/DependenceGraphBuilder.cpp

for (NodeListType &NL : ListOfSCCs) {
     // We have the set of nodes in the SCC. We go through the set of nodes
     // that are outside of the SCC and look for edges that cross the two sets.
     for (NodeType *N : Graph) {
 
       // Skip the SCC node and all the nodes inside of it.
       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};
+      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.");
+        LLVM_DEBUG(
+            dbgs() << "reconnecting("
+                   << ":\nSrc:" << *Src << "\nDst:" << *Dst << "\nNew:" << *New
+                   << "\n");
 
-          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;
-            }
-            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);
 
         // Process edges that are coming out of the pi-block node.
         reconnectEdges(SCCNode, N, &PiNode, Direction::Outgoing);
       }
     }
   }

llvm/unittests/Analysis/DDGTest.cpp

runTest(
         EXPECT_EQ(DL.size(), 1ull);
         EXPECT_TRUE(DL.back()->isAnti());
         EXPECT_EQ(DL.back()->getLevels(), 1u);
         EXPECT_NE(DL.back()->getDistance(1), nullptr);
         EXPECT_EQ(DL.back()->getDistance(1),
                   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);
+      });
+}