[PATCH] D21430: Use a much more efficient (linear instead of quadratic?) algorithm for computing the height: standard DFS mincost algorithm for a DAG.

[Chandler Carruth via llvm-commits]

chandlerc created this revision.
chandlerc added a subscriber: llvm-commits.
Herald added subscribers: mcrosier, MatzeB.

Not only is the old algorithm essentially BFS instead of DFS, it weirdly
can mark heights as dirty which then walks preds. Very strang. I think
a direct DFS that just computes the height of everything in the DAG from
the bottom up and avoids re-computing already computed heights is much
easier to understand.

For test/CodeGen/AMDGPU/spill-scavenge-offset.ll, prior to it having any
scheduler turned off, computing the height was over 25% of the runtime.
With this patch, it is completely gone. I'm measuring improvements from
32s to 24s (25%) in debug builds and 4.5s to 3.17s (30%) in an optimized
build for thes test.

Glancing at it, the depth computation probably needs the same treatment,
but I've not yet found a test that exercises this (I've not looked too
hard yet though).

As with my prior patch, this impacts both SDAG scheduling and MI
scheduling, but for different reasons -- in this case, both schedulers
call the ScheduleDAG's getHeight routine heavily and were causing it
show up in profiles for spill-scavenge-offset.ll.

http://reviews.llvm.org/D21430

Files:
  lib/CodeGen/ScheduleDAG.cpp

Index: lib/CodeGen/ScheduleDAG.cpp
===================================================================
--- lib/CodeGen/ScheduleDAG.cpp
+++ lib/CodeGen/ScheduleDAG.cpp
@@ -290,34 +290,68 @@
 /// ComputeHeight - Calculate the maximal path from the node to the entry.
 ///
 void SUnit::ComputeHeight() {
-  SmallVector<SUnit*, 8> WorkList;
-  WorkList.push_back(this);
-  do {
-    SUnit *Cur = WorkList.back();
+  if (Succs.empty()) {
+    Height = 0;
+    isHeightCurrent = true;
+    return;
+  }
 
-    bool Done = true;
-    unsigned MaxSuccHeight = 0;
-    for (SUnit::const_succ_iterator I = Cur->Succs.begin(),
-         E = Cur->Succs.end(); I != E; ++I) {
+  SmallVector<std::pair<SUnit *, SUnit::const_succ_iterator>, 8> Stack;
+#ifndef NDEBUG
+  SmallPtrSet<SUnit *, 8> Visited;
+  Visited.insert(this);
+#endif
+  Height = 0;
+  SUnit *Cur = this;
+  SUnit::const_succ_iterator I = Succs.begin();
+  for (;;) {
+    assert(!Cur->isHeightCurrent &&
+           "No need to compute the height once current!");
+
+    SUnit::const_succ_iterator E = Cur->Succs.end();
+    assert(I != E && "Got an end iterator into the stack!");
+
+    do {
       SUnit *SuccSU = I->getSUnit();
-      if (SuccSU->isHeightCurrent)
-        MaxSuccHeight = std::max(MaxSuccHeight,
-                                 SuccSU->Height + I->getLatency());
-      else {
-        Done = false;
-        WorkList.push_back(SuccSU);
+      assert(!Visited.count(SuccSU) && "Found a cycle!");
+
+      if (!SuccSU->isHeightCurrent) {
+        if (SuccSU->Succs.empty()) {
+          SuccSU->Height = 0;
+          SuccSU->isHeightCurrent = true;
+        } else {
+          // We have a successor without a current height.
+          // Push the current position onto the stack and process that
+          // successor.
+          Stack.push_back({Cur, I});
+          Cur = SuccSU;
+#ifndef NDEBUG
+          Visited.insert(Cur);
+#endif
+          I = Cur->Succs.begin();
+          E = Cur->Succs.end();
+          continue;
+        }
       }
-    }
 
-    if (Done) {
-      WorkList.pop_back();
-      if (MaxSuccHeight != Cur->Height) {
-        Cur->setHeightDirty();
-        Cur->Height = MaxSuccHeight;
-      }
-      Cur->isHeightCurrent = true;
-    }
-  } while (!WorkList.empty());
+      // Max the current height against this successor's height.
+      Cur->Height = std::max(Cur->Height, SuccSU->Height + I->getLatency());
+      ++I;
+    } while (I != E);
+
+    // We've examined all successors of this SU, our height is now current.
+    Cur->isHeightCurrent = true;
+
+    // If there are no further nodes to process, we're finished.
+    if (Stack.empty())
+      return;
+
+    // Otherwise pop the stack and continue.
+#ifndef NDEBUG
+    Visited.erase(Cur);
+#endif
+    std::tie(Cur, I) = Stack.pop_back_val();
+  }
 }
 
 void SUnit::biasCriticalPath() {


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D21430.60961.patch
Type: text/x-patch
Size: 2866 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20160616/a6cb1a59/attachment.bin>