Addressing PR comments

Author: Bensuo

sycl/source/detail/graph_impl.cpp

@@ -74,8 +74,6 @@ void duplicateNode(const std::shared_ptr<node_impl> Node,
   }
 }
 
-} // anonymous namespace
-
 /// Recursively add nodes to execution stack.
 /// @param NodeImpl Node to schedule.
 /// @param Schedule Execution ordering to add node to.
@@ -90,6 +88,7 @@ void sortTopological(std::shared_ptr<node_impl> NodeImpl,
 
   Schedule.push_front(NodeImpl);
 }
+} // anonymous namespace
 
 void exec_graph_impl::schedule() {
   if (MSchedule.empty()) {
@@ -122,8 +121,7 @@ std::shared_ptr<node_impl> graph_impl::addNodesToExits(
 
   // Find all exit nodes in the current graph and register the Inputs as
   // successors
-  for (size_t i = 0; i < MNodeStorage.size(); i++) {
-    auto NodeImpl = MNodeStorage[i];
+  for (auto &NodeImpl : MNodeStorage) {
     if (NodeImpl->MSuccessors.size() == 0) {
       for (auto &Input : Inputs) {
         NodeImpl->registerSuccessor(Input, NodeImpl);
@@ -184,16 +182,8 @@ graph_impl::add(const std::vector<std::shared_ptr<node_impl>> &Dep) {
   Deps.insert(Deps.end(), MExtraDependencies.begin(), MExtraDependencies.end());
 
   MNodeStorage.push_back(NodeImpl);
-  // TODO: Encapsulate in separate function to avoid duplication
-  if (!Deps.empty()) {
-    for (auto &N : Deps) {
-      N->registerSuccessor(NodeImpl, N); // register successor
-      this->removeRoot(NodeImpl);        // remove receiver from root node
-                                         // list
-    }
-  } else {
-    this->addRoot(NodeImpl);
-  }
+
+  addDepsToNode(NodeImpl, Deps);
 
   return NodeImpl;
 }
@@ -268,7 +258,7 @@ graph_impl::add(sycl::detail::CG::CGTYPE CGType,
       MemObj->markBeingUsedInGraph();
     }
     // Look through the graph for nodes which share this requirement
-    for (auto Node : MNodeStorage) {
+    for (auto &Node : MNodeStorage) {
       if (Node->hasRequirement(Req)) {
         bool ShouldAddDep = true;
         // If any of this node's successors have this requirement then we skip
@@ -304,15 +294,7 @@ graph_impl::add(sycl::detail::CG::CGTYPE CGType,
       std::make_shared<node_impl>(CGType, std::move(CommandGroup));
   MNodeStorage.push_back(NodeImpl);
 
-  if (!Deps.empty()) {
-    for (auto &N : Deps) {
-      N->registerSuccessor(NodeImpl, N); // register successor
-      this->removeRoot(NodeImpl);        // remove receiver from root node
-                                         // list
-    }
-  } else {
-    this->addRoot(NodeImpl);
-  }
+  addDepsToNode(NodeImpl, Deps);
 
   // Set barrier nodes as prerequisites (new start points) for subsequent nodes
   if (CGType == sycl::detail::CG::Barrier) {
@@ -387,7 +369,7 @@ void graph_impl::makeEdge(std::shared_ptr<node_impl> Src,
 
   bool SrcFound = false;
   bool DestFound = false;
-  for (auto Node : MNodeStorage) {
+  for (const auto &Node : MNodeStorage) {
 
     SrcFound |= Node == Src;
     DestFound |= Node == Dest;

sycl/source/detail/graph_impl.hpp

@@ -178,7 +178,7 @@ class node_impl {
     return nullptr;
   }
 
-  /// Tests is the caller is similar to Node, this is only used for testing.
+  /// Tests if the caller is similar to Node, this is only used for testing.
   /// @param Node The node to check for similarity.
   /// @param CompareContentOnly Skip comparisons related to graph structure,
   /// compare only the type and command groups of the nodes
@@ -381,7 +381,10 @@ class graph_impl {
   /// List of root nodes.
   std::set<std::shared_ptr<node_impl>> MRoots;
 
-  /// Storage for all nodes contained within a graph
+  /// Storage for all nodes contained within a graph. Nodes are connected to
+  /// each other via weak_ptrs and so do not extend each other's lifetimes.
+  /// This storage allows easy iteration over all nodes in the graph, rather
+  /// than needing an expensive depth first search.
   std::vector<std::shared_ptr<node_impl>> MNodeStorage;
 
   /// Find the last node added to this graph from an in-order queue.
@@ -430,8 +433,8 @@ class graph_impl {
   /// @param NodeA pointer to the first node for comparison
   /// @param NodeB pointer to the second node for comparison
   /// @return true is same structure found, false otherwise
-  bool checkNodeRecursive(std::shared_ptr<node_impl> NodeA,
-                          std::shared_ptr<node_impl> NodeB) const {
+  static bool checkNodeRecursive(std::shared_ptr<node_impl> NodeA,
+                                 std::shared_ptr<node_impl> NodeB) {
     size_t FoundCnt = 0;
     for (std::weak_ptr<node_impl> &SuccA : NodeA->MSuccessors) {
       for (std::weak_ptr<node_impl> &SuccB : NodeB->MSuccessors) {
@@ -565,6 +568,22 @@ class graph_impl {
   std::shared_ptr<node_impl>
   addNodesToExits(const std::list<std::shared_ptr<node_impl>> &NodeList);
 
+  /// Adds dependencies for a new node, if it has no deps it will be
+  /// added as a root node.
+  /// @param Node The node to add deps for
+  /// @param Deps List of dependent nodes
+  void addDepsToNode(std::shared_ptr<node_impl> Node,
+                     const std::vector<std::shared_ptr<node_impl>> &Deps) {
+    if (!Deps.empty()) {
+      for (auto &N : Deps) {
+        N->registerSuccessor(Node, N);
+        this->removeRoot(Node);
+      }
+    } else {
+      this->addRoot(Node);
+    }
+  }
+
   /// Context associated with this graph.
   sycl::context MContext;
   /// Device associated with this graph. All graph nodes will execute on this