[NFC][SYCL][Graph] Introduce nodes_range utility
#19295
Conversation
aelovikov-intel
had a problem deploying
to
WindowsCILock
GitHub Actions
Error
| N.registerSuccessor(Node); | ||
| this->removeRoot(Node); | ||
| } | ||
| if (Node->MPredecessors.empty()) { |
There was a problem hiding this comment.
IMO, the "root" invariant should be handled completely automatically between node_impl's ctor and add/removePredecessor methods. I'm not familiar with the implementation though, so can't state for certain that it would work and even if so, that should be left to a separate patch.
However, I've changed graph_impl::add overload to stop appending node_impls from std::set to an std::vector and am making to calls to addDepsToNode instead, hence this particular change.
There was a problem hiding this comment.
That refactor of how root nodes gets recording makes, sense but agree that's for a separate patch.
It looks like lots of `std::vector<std::weak_ptr<node_impl>>` 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<node_impl *>`). 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<node_impl *>` passed around as function parameters or created as a local variable on stack, and `std::vector<node>` or `std::vector<std::shared_ptr<node_impl>>` 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.
aelovikov-intel
force-pushed
the
nodes_range
branch
from
e64923e to
bc04d13
Compare
aelovikov-intel
temporarily deployed
to
WindowsCILock
GitHub Actions
Inactive
aelovikov-intel
temporarily deployed
to
WindowsCILock
GitHub Actions
Inactive
aelovikov-intel
had a problem deploying
to
WindowsCILock
GitHub Actions
Failure
aelovikov-intel
temporarily deployed
to
WindowsCILock
GitHub Actions
Inactive
|
For extra context, follow-up changes would look like this: 05d00da (not sure if I'll be able to split in multiple PRs yet). |
| N.registerSuccessor(Node); | ||
| this->removeRoot(Node); | ||
| } | ||
| if (Node->MPredecessors.empty()) { |
There was a problem hiding this comment.
That refactor of how root nodes gets recording makes, sense but agree that's for a separate patch.
It looks like lots of
std::vector<std::weak_ptr<node_impl>>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<node_impl *>). 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_ptrs and that can be seen even as part of this PR already.Scenario A: we need to process both
std::vector<node_impl *>passed around as function parameters or created as a local variable on stack, andstd::vector<node>orstd::vector<std::shared_ptr<node_impl>>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.Another potentially useful application would be to provide "getters" like
nodes_range node_impl::successors()to hide away implementation details of the underlying container.