[PATCH] D30626: MachineScheduler/ScheduleDAG: Add support for GetSubGraph

[Axel Davy via Phabricator via llvm-commits]

axeldavy added inline comments.


================
Comment at: lib/CodeGen/ScheduleDAG.cpp:579
+      // Edges to non-SUnits are allowed but ignored (e.g. ExitSU).
+      if (s >= Node2Index.size())
+        continue;
----------------
MatzeB wrote:
> Better use SUnit::isBoundaryNode()
Thanks, I update the diff with that.


================
Comment at: lib/CodeGen/ScheduleDAG.cpp:587
+      if (!Visited.test(s) && Node2Index[s] < UpperBound)
+        WorkList.push_back(SU->Succs[I].getSUnit());
+    }
----------------
MatzeB wrote:
> Isn't there a mismatch here? I would assume you either test+set visited at the beginning of the loop or you do both when inserting into the worklist.
> 
> Testing before insertion but only setting the visited flag when pulling out of the worklist can lead to situations in which elements are pulled out multiple times.
Right, it sounds better to set Visited directly there.

It may be possible without it items get added several times to the WorkList, though the second time the items would be pulled, the operation would be noop.


================
Comment at: lib/CodeGen/ScheduleDAG.cpp:624
+
+  assert(Found);
+  Success = true;
----------------
hfinkel wrote:
> This assert should have a comment, but instead of tracking Found again, maybe it would be even better to do this:
> 
>   assert(VisitedBack.size() == Visited.size() && "Did not collect all subgraph nodes?");
>  
Thanks, I add a comment in the assert.

However please note that VisitedBack and Visited likely don't have the same size.
Visited can contain more elements.


Repository:
  rL LLVM

https://reviews.llvm.org/D30626