[NFC][SYCL][Graph] Introduce `nodes_range` utility #19295

aelovikov-intel · 2025-07-03T15:54:23Z

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, 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.

Another potentially useful application would be to provide "getters" like
nodes_range node_impl::successors() to hide away implementation details of the underlying container.

aelovikov-intel · 2025-07-03T15:57:18Z

sycl/source/detail/graph/graph_impl.hpp

+      N.registerSuccessor(Node);
+      this->removeRoot(Node);
+    }
+    if (Node->MPredecessors.empty()) {


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.

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.

sycl/source/detail/graph/graph_impl.hpp

aelovikov-intel · 2025-07-03T19:31:41Z

For extra context, follow-up changes would look like this: 05d00da (not sure if I'll be able to split in multiple PRs yet).

EwanC

Your observation that the node weak-ptrs are expected to always be alive is correct. So the move to raw ptrs/reference is a good direction and this is a nice step to get there, thanks.

sycl/source/detail/graph/node_impl.hpp

sycl/source/detail/graph/graph_impl.hpp

EwanC · 2025-07-07T08:15:51Z

sycl/source/detail/graph/graph_impl.hpp

+      N.registerSuccessor(Node);
+      this->removeRoot(Node);
+    }
+    if (Node->MPredecessors.empty()) {


That refactor of how root nodes gets recording makes, sense but agree that's for a separate patch.

Part of refactoring to get rid of most (all?) `std::weak_ptr<node_impl>` and some of `std::shared_ptr<node_impl>` started in intel#19295. Use `nodes_range` from that PR to implement `successors`/`predecessors` views and update read-only accesses to the successors/predecessors to go through them. I'm not changing the data members `MSuccessors`/`MPredecessors` yet because it would affect unittests. I'd prefer to refactor most of the code in future PRs before making that change and updating unittests in one go. I'm updating some APIs to accept `node_impl &` instead of `std::shared_ptr` where the change is mostly localized to the callers iterating over successors/predecessors and doesn't spoil into other code too much. For those that weren't updated here we (temporarily) use `shared_from_this()` but I expect to eliminate those unnecessary copies when those interfaces will be updated in the subsequent PRs.

Continuation of the refactoring in intel#19295 intel#19332

Part of refactoring to get rid of most (all?) `std::weak_ptr<node_impl>` and some of `std::shared_ptr<node_impl>` started in intel#19295. Use `nodes_range` from that PR to implement `successors`/`predecessors` views and update read-only accesses to the successors/predecessors to go through them. I'm not changing the data members `MSuccessors`/`MPredecessors` yet because it would affect unittests. I'd prefer to refactor most of the code in future PRs before making that change and updating unittests in one go. I'm updating some APIs to accept `node_impl &` instead of `std::shared_ptr` where the change is mostly localized to the callers iterating over successors/predecessors and doesn't spoil into other code too much. For those that weren't updated here we (temporarily) use `shared_from_this()` but I expect to eliminate those unnecessary copies when those interfaces will be updated in the subsequent PRs.

Continuation of the refactoring in intel#19295 intel#19332

... and update the code surrounding their uses in the same spirit. Continuation of intel#19295 intel#19332 intel#19334

…19332) Part of refactoring to get rid of most (all?) `std::weak_ptr<node_impl>` and some of `std::shared_ptr<node_impl>` started in #19295. Use `nodes_range` from that PR to implement `successors`/`predecessors` views and update read-only accesses to the successors/predecessors to go through them. I'm not changing the data members `MSuccessors`/`MPredecessors` yet because it would affect unittests. I'd prefer to refactor most of the code in future PRs before making that change and updating unittests in one go. I'm updating some APIs to accept `node_impl &` instead of `std::shared_ptr` where the change is mostly localized to the callers iterating over successors/predecessors and doesn't spoil into other code too much. For those that weren't updated here we (temporarily) use `shared_from_this()` but I expect to eliminate those unnecessary copies when those interfaces will be updated in the subsequent PRs.

... and update the code surrounding their uses in the same spirit. Continuation of intel#19295 intel#19332 intel#19334

aelovikov-intel requested a review from a team as a code owner July 3, 2025 15:54

aelovikov-intel requested a review from EwanC July 3, 2025 15:54

aelovikov-intel had a problem deploying to WindowsCILock July 3, 2025 15:54 — with GitHub Actions Error

aelovikov-intel commented Jul 3, 2025

View reviewed changes

aelovikov-intel force-pushed the nodes_range branch from e64923e to bc04d13 Compare July 3, 2025 16:03

aelovikov-intel temporarily deployed to WindowsCILock July 3, 2025 16:04 — with GitHub Actions Inactive

aelovikov-intel commented Jul 3, 2025

View reviewed changes

sycl/source/detail/graph/graph_impl.hpp Show resolved Hide resolved

aelovikov-intel temporarily deployed to WindowsCILock July 3, 2025 16:31 — with GitHub Actions Inactive

aelovikov-intel had a problem deploying to WindowsCILock July 3, 2025 16:31 — with GitHub Actions Failure

aelovikov-intel temporarily deployed to WindowsCILock July 3, 2025 16:43 — with GitHub Actions Inactive

EwanC approved these changes Jul 7, 2025

View reviewed changes

aelovikov-intel merged commit 322cd13 into intel:sycl Jul 7, 2025
28 of 29 checks passed

aelovikov-intel deleted the nodes_range branch July 7, 2025 15:00

aelovikov-intel mentioned this pull request Jul 7, 2025

[NFC][SYCL][Graph] Add successors/predecessors views + cleanup #19332

Merged

aelovikov-intel added a commit to aelovikov-intel/llvm that referenced this pull request Jul 7, 2025

[NFC][SYCL][Graph] Update some maps to use raw node_impl *

2cad128

Continuation of the refactoring in intel#19295 intel#19332

aelovikov-intel mentioned this pull request Jul 7, 2025

[NFC][SYCL][Graph] Update some maps to use raw node_impl * #19334

Open

aelovikov-intel added a commit to aelovikov-intel/llvm that referenced this pull request Jul 7, 2025

[NFC][SYCL][Graph] Update some maps to use raw node_impl *

4b7e279

Continuation of the refactoring in intel#19295 intel#19332

aelovikov-intel added a commit to aelovikov-intel/llvm that referenced this pull request Jul 7, 2025

[NFC][SYCL][Graph] Update some maps to use raw node_impl *

45c2e52

Continuation of the refactoring in intel#19295 intel#19332

aelovikov-intel added a commit to aelovikov-intel/llvm that referenced this pull request Jul 8, 2025

[NFC][SYCL][Graph] Use raw node_impl * in MRoots/MSchedule

4662cf8

... and update the code surrounding their uses in the same spirit. Continuation of intel#19295 intel#19332 intel#19334

aelovikov-intel added a commit to aelovikov-intel/llvm that referenced this pull request Jul 8, 2025

[NFC][SYCL][Graph] Use raw node_impl * in MRoots/MSchedule

954dba4

... and update the code surrounding their uses in the same spirit. Continuation of intel#19295 intel#19332 intel#19334

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[NFC][SYCL][Graph] Introduce `nodes_range` utility #19295

[NFC][SYCL][Graph] Introduce `nodes_range` utility #19295

Uh oh!

aelovikov-intel commented Jul 3, 2025 •

edited

Loading

Uh oh!

aelovikov-intel Jul 3, 2025

Uh oh!

EwanC Jul 7, 2025

Uh oh!

Uh oh!

aelovikov-intel commented Jul 3, 2025 •

edited

Loading

Uh oh!

EwanC left a comment

Uh oh!

Uh oh!

Uh oh!

EwanC Jul 7, 2025

Uh oh!

Uh oh!

Uh oh!

[NFC][SYCL][Graph] Introduce nodes_range utility #19295

[NFC][SYCL][Graph] Introduce nodes_range utility #19295

Uh oh!

Conversation

aelovikov-intel commented Jul 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aelovikov-intel Jul 3, 2025

Choose a reason for hiding this comment

Uh oh!

EwanC Jul 7, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

aelovikov-intel commented Jul 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

EwanC left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

EwanC Jul 7, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

[NFC][SYCL][Graph] Introduce `nodes_range` utility #19295

[NFC][SYCL][Graph] Introduce `nodes_range` utility #19295

aelovikov-intel commented Jul 3, 2025 •

edited

Loading

aelovikov-intel commented Jul 3, 2025 •

edited

Loading