From f04c79b78a825d9a0d9d002d66ad53749e94a16e Mon Sep 17 00:00:00 2001 From: Ewan Crawford Date: Fri, 30 Aug 2024 09:54:55 +0100 Subject: [PATCH] [SYCL][Graph] `enable_shared_from_this` refactor (#15195) Use `std::enable_shared_from_this` to remove need for passing a shared pointer of `this` as a function parameter. `std::enable_shared_from_this` usage was previously introduced to graph code in https://github.com/intel/llvm/pull/14453#discussion_r1681412183 --- sycl/source/detail/graph_impl.cpp | 39 ++++++++++++---------------- sycl/source/detail/graph_impl.hpp | 43 ++++++++++--------------------- sycl/source/handler.cpp | 2 +- 3 files changed, 32 insertions(+), 52 deletions(-) diff --git a/sycl/source/detail/graph_impl.cpp b/sycl/source/detail/graph_impl.cpp index 6836a86010cc4..c876942c2ce3b 100644 --- a/sycl/source/detail/graph_impl.cpp +++ b/sycl/source/detail/graph_impl.cpp @@ -308,7 +308,6 @@ graph_impl::~graph_impl() { } std::shared_ptr graph_impl::addNodesToExits( - const std::shared_ptr &Impl, const std::list> &NodeList) { // Find all input and output nodes from the node list std::vector> Inputs; @@ -327,7 +326,7 @@ std::shared_ptr graph_impl::addNodesToExits( for (auto &NodeImpl : MNodeStorage) { if (NodeImpl->MSuccessors.size() == 0) { for (auto &Input : Inputs) { - NodeImpl->registerSuccessor(Input, NodeImpl); + NodeImpl->registerSuccessor(Input); } } } @@ -335,10 +334,10 @@ std::shared_ptr graph_impl::addNodesToExits( // Add all the new nodes to the node storage for (auto &Node : NodeList) { MNodeStorage.push_back(Node); - addEventForNode(Impl, std::make_shared(), Node); + addEventForNode(std::make_shared(), Node); } - return this->add(Impl, Outputs); + return this->add(Outputs); } void graph_impl::addRoot(const std::shared_ptr &Root) { @@ -350,8 +349,7 @@ void graph_impl::removeRoot(const std::shared_ptr &Root) { } std::shared_ptr -graph_impl::add(const std::shared_ptr &Impl, - const std::vector> &Dep) { +graph_impl::add(const std::vector> &Dep) { // Copy deps so we can modify them auto Deps = Dep; @@ -361,17 +359,16 @@ graph_impl::add(const std::shared_ptr &Impl, addDepsToNode(NodeImpl, Deps); // Add an event associated with this explicit node for mixed usage - addEventForNode(Impl, std::make_shared(), NodeImpl); + addEventForNode(std::make_shared(), NodeImpl); return NodeImpl; } std::shared_ptr -graph_impl::add(const std::shared_ptr &Impl, - std::function CGF, +graph_impl::add(std::function CGF, const std::vector &Args, const std::vector> &Dep) { (void)Args; - sycl::handler Handler{Impl}; + sycl::handler Handler{shared_from_this()}; CGF(Handler); if (Handler.getType() == sycl::detail::CGType::Barrier) { @@ -394,7 +391,7 @@ graph_impl::add(const std::shared_ptr &Impl, this->add(NodeType, std::move(Handler.impl->MGraphNodeCG), Dep); NodeImpl->MNDRangeUsed = Handler.impl->MNDRangeUsed; // Add an event associated with this explicit node for mixed usage - addEventForNode(Impl, std::make_shared(), NodeImpl); + addEventForNode(std::make_shared(), NodeImpl); // Retrieve any dynamic parameters which have been registered in the CGF and // register the actual nodes with them. @@ -414,8 +411,7 @@ graph_impl::add(const std::shared_ptr &Impl, } std::shared_ptr -graph_impl::add(const std::shared_ptr &Impl, - const std::vector Events) { +graph_impl::add(const std::vector Events) { std::vector> Deps; @@ -430,7 +426,7 @@ graph_impl::add(const std::shared_ptr &Impl, } } - return this->add(Impl, Deps); + return this->add(Deps); } std::shared_ptr @@ -594,7 +590,7 @@ void graph_impl::makeEdge(std::shared_ptr Src, } // We need to add the edges first before checking for cycles - Src->registerSuccessor(Dest, Src); + Src->registerSuccessor(Dest); // We can skip cycle checks if either Dest has no successors (cycle not // possible) or cycle checks have been disabled with the no_cycle_check @@ -1061,7 +1057,7 @@ void exec_graph_impl::duplicateNodes() { // register those as successors with the current copied node for (auto &NextNode : OriginalNode->MSuccessors) { auto Successor = NodesMap.at(NextNode.lock()); - NodeCopy->registerSuccessor(Successor, NodeCopy); + NodeCopy->registerSuccessor(Successor); } } @@ -1103,7 +1099,7 @@ void exec_graph_impl::duplicateNodes() { for (auto &NextNode : SubgraphNode->MSuccessors) { auto Successor = SubgraphNodesMap.at(NextNode.lock()); - NodeCopy->registerSuccessor(Successor, NodeCopy); + NodeCopy->registerSuccessor(Successor); } } @@ -1137,7 +1133,7 @@ void exec_graph_impl::duplicateNodes() { // Add all input nodes from the subgraph as successors for this node // instead for (auto &Input : Inputs) { - PredNode->registerSuccessor(Input, PredNode); + PredNode->registerSuccessor(Input); } } @@ -1157,7 +1153,7 @@ void exec_graph_impl::duplicateNodes() { // Add all Output nodes from the subgraph as predecessors for this node // instead for (auto &Output : Outputs) { - Output->registerSuccessor(SuccNode, Output); + Output->registerSuccessor(SuccNode); } } @@ -1531,7 +1527,7 @@ node modifiable_command_graph::addImpl(const std::vector &Deps) { } graph_impl::WriteLock Lock(impl->MMutex); - std::shared_ptr NodeImpl = impl->add(impl, DepImpls); + std::shared_ptr NodeImpl = impl->add(DepImpls); return sycl::detail::createSyclObjFromImpl(NodeImpl); } @@ -1544,8 +1540,7 @@ node modifiable_command_graph::addImpl(std::function CGF, } graph_impl::WriteLock Lock(impl->MMutex); - std::shared_ptr NodeImpl = - impl->add(impl, CGF, {}, DepImpls); + std::shared_ptr NodeImpl = impl->add(CGF, {}, DepImpls); return sycl::detail::createSyclObjFromImpl(NodeImpl); } diff --git a/sycl/source/detail/graph_impl.hpp b/sycl/source/detail/graph_impl.hpp index 4f0d1935ab615..4844208d69010 100644 --- a/sycl/source/detail/graph_impl.hpp +++ b/sycl/source/detail/graph_impl.hpp @@ -77,7 +77,7 @@ inline node_type getNodeTypeFromCG(sycl::detail::CGType CGType) { } /// Implementation of node class from SYCL_EXT_ONEAPI_GRAPH. -class node_impl { +class node_impl : public std::enable_shared_from_this { public: using id_type = uint64_t; @@ -112,12 +112,7 @@ class node_impl { /// Add successor to the node. /// @param Node Node to add as a successor. - /// @param Prev Predecessor to \p node being added as successor. - /// - /// \p Prev should be a shared_ptr to an instance of this object, but can't - /// use a raw \p this pointer, so the extra \p Prev parameter is passed. - void registerSuccessor(const std::shared_ptr &Node, - const std::shared_ptr &Prev) { + void registerSuccessor(const std::shared_ptr &Node) { if (std::find_if(MSuccessors.begin(), MSuccessors.end(), [Node](const std::weak_ptr &Ptr) { return Ptr.lock() == Node; @@ -125,7 +120,7 @@ class node_impl { return; } MSuccessors.push_back(Node); - Node->registerPredecessor(Prev); + Node->registerPredecessor(shared_from_this()); } /// Add predecessor to the node. @@ -161,9 +156,10 @@ class node_impl { /// Construct a node from another node. This will perform a deep-copy of the /// command group object associated with this node. node_impl(node_impl &Other) - : MSuccessors(Other.MSuccessors), MPredecessors(Other.MPredecessors), - MCGType(Other.MCGType), MNodeType(Other.MNodeType), - MCommandGroup(Other.getCGCopy()), MSubGraphImpl(Other.MSubGraphImpl) {} + : enable_shared_from_this(Other), MSuccessors(Other.MSuccessors), + MPredecessors(Other.MPredecessors), MCGType(Other.MCGType), + MNodeType(Other.MNodeType), MCommandGroup(Other.getCGCopy()), + MSubGraphImpl(Other.MSubGraphImpl) {} /// Copy-assignment operator. This will perform a deep-copy of the /// command group object associated with this node. @@ -901,32 +897,26 @@ class graph_impl : public std::enable_shared_from_this { const std::vector> &Dep = {}); /// Create a CGF node in the graph. - /// @param Impl Graph implementation pointer to create a handler with. /// @param CGF Command-group function to create node with. /// @param Args Node arguments. /// @param Dep Dependencies of the created node. /// @return Created node in the graph. std::shared_ptr - add(const std::shared_ptr &Impl, - std::function CGF, + add(std::function CGF, const std::vector &Args, const std::vector> &Dep = {}); /// Create an empty node in the graph. - /// @param Impl Graph implementation pointer. /// @param Dep List of predecessor nodes. /// @return Created node in the graph. std::shared_ptr - add(const std::shared_ptr &Impl, - const std::vector> &Dep = {}); + add(const std::vector> &Dep = {}); /// Create an empty node in the graph. - /// @param Impl Graph implementation pointer. /// @param Events List of events associated to this node. /// @return Created node in the graph. std::shared_ptr - add(const std::shared_ptr &Impl, - const std::vector Events); + add(const std::vector Events); /// Add a queue to the set of queues which are currently recording to this /// graph. @@ -951,15 +941,12 @@ class graph_impl : public std::enable_shared_from_this { bool clearQueues(); /// Associate a sycl event with a node in the graph. - /// @param GraphImpl shared_ptr to Graph impl associated with this event, aka - /// this. /// @param EventImpl Event to associate with a node in map. /// @param NodeImpl Node to associate with event in map. - void addEventForNode(std::shared_ptr GraphImpl, - std::shared_ptr EventImpl, + void addEventForNode(std::shared_ptr EventImpl, std::shared_ptr NodeImpl) { if (!(EventImpl->getCommandGraph())) - EventImpl->setCommandGraph(GraphImpl); + EventImpl->setCommandGraph(shared_from_this()); MEventsMap[EventImpl] = NodeImpl; } @@ -1238,12 +1225,10 @@ class graph_impl : public std::enable_shared_from_this { void addRoot(const std::shared_ptr &Root); /// Adds nodes to the exit nodes of this graph. - /// @param Impl Graph implementation pointer. /// @param NodeList List of nodes from sub-graph in schedule order. /// @return An empty node is used to schedule dependencies on this sub-graph. std::shared_ptr - addNodesToExits(const std::shared_ptr &Impl, - const std::list> &NodeList); + addNodesToExits(const std::list> &NodeList); /// Adds dependencies for a new node, if it has no deps it will be /// added as a root node. @@ -1253,7 +1238,7 @@ class graph_impl : public std::enable_shared_from_this { const std::vector> &Deps) { if (!Deps.empty()) { for (auto &N : Deps) { - N->registerSuccessor(Node, N); + N->registerSuccessor(Node); this->removeRoot(Node); } } else { diff --git a/sycl/source/handler.cpp b/sycl/source/handler.cpp index 5ceb4724d3485..dffea5915241e 100644 --- a/sycl/source/handler.cpp +++ b/sycl/source/handler.cpp @@ -573,7 +573,7 @@ event handler::finalize() { } // Associate an event with this new node and return the event. - GraphImpl->addEventForNode(GraphImpl, EventImpl, NodeImpl); + GraphImpl->addEventForNode(EventImpl, NodeImpl); NodeImpl->MNDRangeUsed = impl->MNDRangeUsed;