From bc04d1379e3bfd8079be229306e66d475e81e597 Mon Sep 17 00:00:00 2001 From: Andrei Elovikov Date: Wed, 2 Jul 2025 14:36:52 -0700 Subject: [PATCH] [NFC][SYCL][Graph] Introduce `nodes_range` utility It looks like lots of `std::vector>` aren't actually used for lifetime management and aren't expected to ever have an "expired" `std::weak_ptr`. As such, it would make sense to transition many of them to raw pointers/references (e.g. `std::vector`). This utility should help to make such transition a bit easier. However, I expect it to be useful even after the elimination of unnecessary `weak_ptr`s and that can be seen even as part of this PR already. * Scenario A: we need to process both `std::vector` passed around as function parameters or created as a local variable on stack, and `std::vector` or `std::vector>` that *is* used for lifetime management (e.g. `handler`'s data member, or all the nodes in the graph). This utility would allow such an API to have a single overload to work with both scenarios. * Scenario B: no conversion between `std::set`->`std::vector<>` (already updated here) is necessary and no templates required to support them both via single overload. --- sycl/source/detail/graph/graph_impl.cpp | 18 +++--- sycl/source/detail/graph/graph_impl.hpp | 21 +++--- sycl/source/detail/graph/node_impl.hpp | 85 +++++++++++++++++++++++-- 3 files changed, 98 insertions(+), 26 deletions(-) diff --git a/sycl/source/detail/graph/graph_impl.cpp b/sycl/source/detail/graph/graph_impl.cpp index 7e9be4a7f889a..9af405c9a86b0 100644 --- a/sycl/source/detail/graph/graph_impl.cpp +++ b/sycl/source/detail/graph/graph_impl.cpp @@ -452,8 +452,7 @@ void graph_impl::markCGMemObjs( } } -std::shared_ptr -graph_impl::add(std::vector> &Deps) { +std::shared_ptr graph_impl::add(nodes_range Deps) { const std::shared_ptr &NodeImpl = std::make_shared(); MNodeStorage.push_back(NodeImpl); @@ -542,12 +541,12 @@ graph_impl::add(std::function CGF, std::shared_ptr graph_impl::add(const std::vector Events) { - std::vector> Deps; + std::vector Deps; // Add any nodes specified by event dependencies into the dependency list for (const auto &Dep : Events) { if (auto NodeImpl = MEventsMap.find(Dep); NodeImpl != MEventsMap.end()) { - Deps.push_back(NodeImpl->second); + Deps.push_back(NodeImpl->second.get()); } else { throw sycl::exception(sycl::make_error_code(errc::invalid), "Event dependency from handler::depends_on does " @@ -561,7 +560,7 @@ graph_impl::add(const std::vector Events) { std::shared_ptr graph_impl::add(node_type NodeType, std::shared_ptr CommandGroup, - std::vector> &Deps) { + nodes_range Deps) { // A unique set of dependencies obtained by checking requirements and events std::set> UniqueDeps = getCGEdges(CommandGroup); @@ -569,15 +568,14 @@ graph_impl::add(node_type NodeType, // Track and mark the memory objects being used by the graph. markCGMemObjs(CommandGroup); - // Add any deps determined from requirements and events into the dependency - // list - Deps.insert(Deps.end(), UniqueDeps.begin(), UniqueDeps.end()); - const std::shared_ptr &NodeImpl = std::make_shared(NodeType, std::move(CommandGroup)); MNodeStorage.push_back(NodeImpl); + // Add any deps determined from requirements and events into the dependency + // list addDepsToNode(NodeImpl, Deps); + addDepsToNode(NodeImpl, UniqueDeps); if (NodeType == node_type::async_free) { auto AsyncFreeCG = @@ -592,7 +590,7 @@ graph_impl::add(node_type NodeType, std::shared_ptr graph_impl::add(std::shared_ptr &DynCGImpl, - std::vector> &Deps) { + nodes_range Deps) { // Set of Dependent nodes based on CG event and accessor dependencies. std::set> DynCGDeps = getCGEdges(DynCGImpl->MCommandGroups[0]); diff --git a/sycl/source/detail/graph/graph_impl.hpp b/sycl/source/detail/graph/graph_impl.hpp index 2b3c422479647..8c22c6fbb754f 100644 --- a/sycl/source/detail/graph/graph_impl.hpp +++ b/sycl/source/detail/graph/graph_impl.hpp @@ -147,7 +147,7 @@ class graph_impl : public std::enable_shared_from_this { /// @return Created node in the graph. std::shared_ptr add(node_type NodeType, std::shared_ptr CommandGroup, - std::vector> &Deps); + nodes_range Deps); /// Create a CGF node in the graph. /// @param CGF Command-group function to create node with. @@ -161,7 +161,7 @@ class graph_impl : public std::enable_shared_from_this { /// Create an empty node in the graph. /// @param Deps List of predecessor nodes. /// @return Created node in the graph. - std::shared_ptr add(std::vector> &Deps); + std::shared_ptr add(nodes_range Deps); /// Create an empty node in the graph. /// @param Events List of events associated to this node. @@ -174,8 +174,7 @@ class graph_impl : public std::enable_shared_from_this { /// @param Deps List of predecessor nodes. /// @return Created node in the graph. std::shared_ptr - add(std::shared_ptr &DynCGImpl, - std::vector> &Deps); + add(std::shared_ptr &DynCGImpl, nodes_range Deps); /// Add a queue to the set of queues which are currently recording to this /// graph. @@ -542,14 +541,12 @@ class graph_impl : public std::enable_shared_from_this { /// added as a root node. /// @param Node The node to add deps for /// @param Deps List of dependent nodes - void addDepsToNode(const std::shared_ptr &Node, - std::vector> &Deps) { - if (!Deps.empty()) { - for (auto &N : Deps) { - N->registerSuccessor(Node); - this->removeRoot(Node); - } - } else { + void addDepsToNode(const std::shared_ptr &Node, nodes_range Deps) { + for (node_impl &N : Deps) { + N.registerSuccessor(Node); + this->removeRoot(Node); + } + if (Node->MPredecessors.empty()) { this->addRoot(Node); } } diff --git a/sycl/source/detail/graph/node_impl.hpp b/sycl/source/detail/graph/node_impl.hpp index f6337b15f4407..455d3b921fd20 100644 --- a/sycl/source/detail/graph/node_impl.hpp +++ b/sycl/source/detail/graph/node_impl.hpp @@ -14,10 +14,11 @@ #include // for CGType #include // for kernel_param_kind_t -#include // for memcpy -#include // for fstream, ostream -#include // for setw, setfill -#include // for vector +#include +#include +#include +#include +#include namespace sycl { inline namespace _V1 { @@ -753,6 +754,82 @@ class node_impl : public std::enable_shared_from_this { return std::make_unique(*static_cast(MCommandGroup.get())); } }; + +// Non-owning! +class nodes_range { + template + using storage_iter_impl = + std::variant; + + using storage_iter = storage_iter_impl< + std::vector>, std::vector, + // Next one is temporary. It looks like `weak_ptr`s aren't + // used for the actual lifetime management and the objects are + // always guaranteed to be alive. Once the code is cleaned + // from `weak_ptr`s this alternative should be removed too. + std::vector>, + // + std::set>>; + + storage_iter Begin; + storage_iter End; + const size_t Size; + +public: + nodes_range(const nodes_range &Other) = default; + + template < + typename ContainerTy, + typename = std::enable_if_t>> + nodes_range(ContainerTy &Container) + : Begin{Container.begin()}, End{Container.end()}, Size{Container.size()} { + } + + class iterator { + storage_iter It; + + iterator(storage_iter It) : It(It) {} + friend class nodes_range; + + public: + iterator &operator++() { + It = std::visit( + [](auto &&It) { + ++It; + return storage_iter{It}; + }, + It); + return *this; + } + bool operator!=(const iterator &Other) const { return It != Other.It; } + + node_impl &operator*() { + return std::visit( + [](auto &&It) -> node_impl & { + auto &Elem = *It; + if constexpr (std::is_same_v, + std::weak_ptr>) { + // This assumes that weak_ptr doesn't actually manage lifetime and + // the object is guaranteed to be alive (which seems to be the + // assumption across all graph code). + return *Elem.lock(); + } else { + return *Elem; + } + }, + It); + } + }; + + iterator begin() const { + return {std::visit([](auto &&It) { return storage_iter{It}; }, Begin)}; + } + iterator end() const { + return {std::visit([](auto &&It) { return storage_iter{It}; }, End)}; + } + size_t size() const { return Size; } + bool empty() const { return Size == 0; } +}; } // namespace detail } // namespace experimental } // namespace oneapi