Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[SYCL][Graph] Skip Graph tests based on sycl-ls output #12812

Merged
merged 7 commits into from
Mar 11, 2024

Conversation

EwanC
Copy link
Contributor

@EwanC EwanC commented Feb 23, 2024

The graph extension tests are currently skipped during execution for devices which don't support the graphs extension. However, this early return causes the tests to be reported as passed and makes it hard from looking at the results to know if the tests actually stressed the graphs code or not.

Improved this situation by changing the SYCL-Graph device info query to an aspect such that sycl-ls --verbose will output ext_oneapi_graph for supported devices. This can then be used to inform the LIT config and set a requirement for tests, enabling the tests to be obviously skipped for devices that don't support graphs.

To enable setting this requirement in lit.local.cfg files some extra directories have been created, in particular UnsupportedDevice which contains tests that don't have a requirement as the tests verify expected errors are thrown when using the graphs API with unsupported devices.

The removal of the device info query means that we can no longer report if a device emulates support for SYCL-Graph, however we currently have no such implementations as they haven't yet deemed to provide enough value. This is technically an ABI breaking change however due to the removal of symbols, but SYCL-Graph is currently an experimental extension so such changes may be permitted.

@bader
Copy link
Contributor

bader commented Feb 23, 2024

Hint.
@EwanC, in case the intention is to skip all tests in sycl/test-e2e/Graph/ directory it can be done by marking the whole directory as "unsupported" in sycl/test-e2e/Graph/lit.local.cfg.

Here is an example: https://github.com/intel/llvm/blob/sycl/clang/test/Interpreter/CUDA/lit.local.cfg. All tests from clang/test/Interpreter/CUDA/ directory are skipped if host doesn't support CUDA.

@EwanC EwanC marked this pull request as ready for review February 27, 2024 08:50
@EwanC EwanC requested review from a team as code owners February 27, 2024 08:50
Copy link
Contributor

@mfrancepillois mfrancepillois left a comment

Choose a reason for hiding this comment

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

LGTM. Just minor comments.

sycl/test-e2e/Graph/Error/empty_graph.cpp Outdated Show resolved Hide resolved
sycl/test-e2e/Graph/Error/finalize_twice.cpp Outdated Show resolved Hide resolved
sycl/test-e2e/Graph/RecordReplay/event_profiling_info.cpp Outdated Show resolved Hide resolved
Copy link
Contributor

@reble reble left a comment

Choose a reason for hiding this comment

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

LGTM

@EwanC
Copy link
Contributor Author

EwanC commented Feb 29, 2024

@intel/llvm-reviewers-runtime Could I get a review on this please

Copy link
Contributor

@steffenlarsen steffenlarsen left a comment

Choose a reason for hiding this comment

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

Changes LGTM! Since it is now a change to the extension however, I would like @reble, @Bensuo, and/or @mfrancepillois to re-approve it.

sycl/include/sycl/info/info_desc.hpp Show resolved Hide resolved
Also check for OpenCL backend support for
`cl_khr_command_buffer` when determining if
SYCL-Graphs support should be reported.
@EwanC
Copy link
Contributor Author

EwanC commented Mar 6, 2024

ping @intel/dpcpp-tools-reviewers for review

@EwanC
Copy link
Contributor Author

EwanC commented Mar 11, 2024

@AlexeySachkov @sarnex @MrSidims Could one of you review on behalf of @intel/dpcpp-tools-reviewers please

Copy link
Contributor

@AlexeySachkov AlexeySachkov left a comment

Choose a reason for hiding this comment

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

DeviceConfigFile.td LGTM, apology for the delay

@EwanC
Copy link
Contributor Author

EwanC commented Mar 11, 2024

@intel/llvm-gatekeepers This is good to merge now

@dm-vodopyanov dm-vodopyanov merged commit a6301e9 into intel:sycl Mar 11, 2024
13 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.

10 participants