Skip to content

[SYCL][Graph] Add get_id() public member function to modifiable command graph#22497

Merged
KornevNikita merged 13 commits into
intel:syclfrom
adamfidel:matt/capture-id-native-recording
Jul 6, 2026
Merged

[SYCL][Graph] Add get_id() public member function to modifiable command graph#22497
KornevNikita merged 13 commits into
intel:syclfrom
adamfidel:matt/capture-id-native-recording

Conversation

@mmichel11

Copy link
Copy Markdown
Contributor
  • Follow-up to [SYCL][Graph] Add get_id() public method for graph #22194 with native recording support.
  • Adds monotonically increasing graph IDs per process. Graph ID uniqueness is guaranteed in an application for the set of graphs using native recording and additionally for the set of graphs not using native recording. The application is required to differentiate if they intermix backends and require uniqueness.
  • For native recording backends, IDs are fetched from the native API. If graph ID support is not present, then a fallback to a SYCL graph internal counter is used.

@mmichel11 mmichel11 marked this pull request as ready for review June 30, 2026 21:07
@mmichel11 mmichel11 requested review from a team as code owners June 30, 2026 21:07
}

unsigned long long getID() const { return MID; }
unsigned long long getID() const { return MNativeID.value_or(MID); }

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the case where the application intermixes command buffer and native graphs, the command buffer IDs will increase monotonically (satisfying the spec) but not necessarily by increments of 1. This is because MID is still used for hashing by both native graphs and command buffer and will increment when either graph type is created.

@adamfidel If this is undesirable and we want command buffer IDs to increment by 1 for this case, then I think we can add a separate counter for the public facing ID.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this behavior is fine for a few reasons:

  1. In the spec, it says "IDs increase monotonically as graph objects are created." Because we do not state that IDs increase monotonically when graph objects of the same recording type are created, then I believe this behavior is spec-compliant according to the proposal.
  2. We already provided caveats if the user decides to mix graphs of various recording types
  3. We anticipate that this use case of mixing native graphs with non-native graphs in the same application would be rare

So I think this implementation is fine as is.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@adamfidel since you expect it to be spec complaint, would it be better to extend spec with the clarification that granularity of monotonical increase for mixed cases are implementation-defined?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A monotonically increasing function has no guarantees regarding the amount of increase, but I agree it may be good to clarify it for users less familiar with the term. I added a sentence describing this.

L0 native graph also has instances where the ID increase is not 1 when graph fork-join occurs. It is good to make sure this is highlighted so users do not try to use it as a vector index for instance.

@adamfidel adamfidel left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@mmichel11

Copy link
Copy Markdown
Contributor Author

@KseniyaTikhomirova @intel/llvm-reviewers-runtime Could you please take a look at this PR?

CI failures are unrelated:

  • Graph/Update/RecordReplay/whole_update_host_task.cpp is flaky and failing in other branches. Will disable this separately.
  • bindless_images/dx11_interop/read_write_unsampled.cpp and Regression/multi_thread_enqueue_discarded.cpp are unrelated tests.

Comment thread sycl/doc/extensions/experimental/sycl_ext_oneapi_graph.asciidoc
Comment thread sycl/source/detail/graph/graph_impl.hpp

@KseniyaTikhomirova KseniyaTikhomirova left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, just a couple of nits

@mmichel11

Copy link
Copy Markdown
Contributor Author

@intel/llvm-gatekeepers Please consider merging.

@KornevNikita KornevNikita merged commit 5133bc6 into intel:sycl Jul 6, 2026
31 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants