[PATCH] Add control dependence computation that computes control dependence equivalence classes.

[Chandler Carruth]

I can't comment on the algorithmic side, but am happy to review the basic code.

First comment would be that if this is really only providing equivalence classes, it should be named appropriately. Probably ControlDependenceEquivalence or some such. Do that last though so the in-line comments aren't lost.


================
Comment at: include/llvm/Analysis/ControlDependence.h:9-12
@@ +8,6 @@
+//===----------------------------------------------------------------------===//
+// \file
+// \brief Determines control dependence equivalence classes for basic
+// blocks. Any two blocks having the same set of control dependences land in one
+// class.
+// ===---------------------------------------------------------------------===//
----------------
You need three '/'s for the doxygen stuff.

================
Comment at: include/llvm/Analysis/ControlDependence.h:59-64
@@ +58,8 @@
+
+  /// \brief Return the control dependence class number for \p BB.  All basic
+  /// blocks with the same class number have the same control dependences
+  unsigned getClassNumber(BasicBlock *BB) {
+    assert(Computed && "Trying to get equivalence classes before computation");
+    return BlockInfo[BB].ClassNumber;
+  }
+
----------------
Is 'unsigned' enough? (I can believe it is, I just want to surface the question.)

================
Comment at: lib/Analysis/ControlDependence.cpp:21-24
@@ +20,6 @@
+using namespace llvm;
+char ControlDependence::ID = 0;
+#define DEBUG_TYPE "controlequiv"
+INITIALIZE_PASS(ControlDependence, "controldep",
+                "Control Dependence Construction", true, true);
+
----------------
Why are these jammed in here with headers and usings?

================
Comment at: lib/Analysis/ControlDependence.cpp:29
@@ +28,3 @@
+// numbers for all participating blocks. Takes O(E) time and O(N) space.
+
+bool ControlDependence::runOnFunction(Function &F) {
----------------
Weird whitspace...

================
Comment at: lib/Analysis/ControlDependence.cpp:122-134
@@ +121,15 @@
+        // and preorder-visit
+        if (PredData.OnStack) {
+          DEBUG(dbgs() << "Maybe visit pred backedge\n");
+          if (Pred != Entry.ParentBlock) {
+            BlockInfo[B].Backedges.push_back(Pred);
+            visitBackedge(B, Pred, PredDirection);
+          }
+
+        } else {
+          BlockInfo[B].Children.push_back(Pred);
+          pushDFS(Stack, Pred, B, PredDirection);
+          visitPre(Pred);
+        }
+        continue;
+      }
----------------
This looks like it could be cleaned up with early-continue patterns.

================
Comment at: lib/Analysis/ControlDependence.cpp:200-201
@@ +199,4 @@
+      combined_succ_end(B, BlockResultData.FakeSuccEdges), From, B});
+}
+/// \brief Pop an entry from the DFS stack.
+void ControlDependence::popDFS(DFSStack &Stack, const BasicBlock *B) {
----------------
Missing vertical whitespace.

================
Comment at: lib/Analysis/ControlDependence.cpp:260-261
@@ +259,4 @@
+      BII = Info.BracketIterators.erase(BII);
+    } else
+      ++BII;
+  }
----------------
Keep braces symmetric.

================
Comment at: lib/Analysis/ControlDependence.cpp:311-321
@@ +310,13 @@
+    else {
+      // Get the min and the max through our children
+      auto hi1iter = std::min_element(
+          Info.Children.begin(), Info.Children.end(),
+          [this](const BasicBlock *A, const BasicBlock *B) {
+            return BlockInfo[A].DFSNumber < BlockInfo[B].DFSNumber;
+          });
+      auto hi2iter = std::max_element(
+          Info.Children.begin(), Info.Children.end(),
+          [this](const BasicBlock *A, const BasicBlock *B) {
+            return BlockInfo[A].DFSNumber < BlockInfo[B].DFSNumber;
+          });
+      Hi1 = *hi1iter;
----------------
Variable naming: Hi1Iter

================
Comment at: lib/Analysis/ControlDependence.cpp:376-377
@@ +375,4 @@
+  DEBUG(dbgs() << "Assigned class number is " << Info.ClassNumber << "\n");
+}
+void ControlDependence::visitPost(const BasicBlock *B,
+                                  const BasicBlock *ParentBlock,
----------------
Again, missing whitespace.

================
Comment at: lib/Analysis/ControlDependence.cpp:390-402
@@ +389,15 @@
+
+  // By the time we hit this node, we are guaranteed these iterators will point
+  // into our list, because it must have been spliced into us.
+  for (auto BII = Info.BracketIterators.begin(),
+            BIE = Info.BracketIterators.end();
+       BII != BIE;) {
+    BracketList::iterator &BLI = *BII;
+    Bracket &BR = *BLI;
+    if (BR.To == B && BR.Direction != Direction) {
+      Info.BList.erase(BLI);
+      BII = Info.BracketIterators.erase(BII);
+    } else
+      ++BII;
+  }
+
----------------
This code seems completely duplicated with the above?

================
Comment at: lib/Analysis/ControlDependence.cpp:410
@@ +409,3 @@
+    BracketList &ParentBList = BlockInfo[ParentBlock].BList;
+    // TODO
+    ParentBList.splice(ParentBList.end(), BList);
----------------
TODO what?

================
Comment at: lib/Analysis/ControlDependence.cpp:433-434
@@ +432,4 @@
+  BlockInfo[To].BracketIterators.push_back(Place);
+}
+FunctionPass *llvm::createControlDependencePass() {
+  return new ControlDependence();
----------------
Missing whitespace.

http://reviews.llvm.org/D8568

EMAIL PREFERENCES
  http://reviews.llvm.org/settings/panel/emailpreferences/