Skip to content

[SYCL] Add device config file consistency test #16369

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 27 commits into from
Jun 11, 2025
Merged

[SYCL] Add device config file consistency test #16369

merged 27 commits into from
Jun 11, 2025

Conversation

jzc
Copy link
Contributor

@jzc jzc commented Dec 13, 2024

No description provided.

@jzc jzc requested review from a team as code owners December 13, 2024 23:06
@jzc jzc requested a review from maarquitos14 December 13, 2024 23:06
@jzc jzc temporarily deployed to WindowsCILock December 13, 2024 23:07 — with GitHub Actions Inactive
@jzc jzc requested a review from a team as a code owner December 19, 2024 18:04
sarnex
sarnex reviewed Dec 19, 2024
View reviewed changes
sycl/CMakeLists.txt
@@ -532,6 +532,11 @@ if("hip" IN_LIST SYCL_ENABLE_BACKENDS)
list(APPEND SYCL_TOOLCHAIN_DEPLOY_COMPONENTS ur_adapter_hip)
endif()

if(INSTALL_DEVICE_CONFIG_FILE)
add_dependencies(sycl-toolchain DeviceConfigFile)
Copy link
Contributor

Choose a reason for hiding this comment

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

can you give some background on why we need to install this? thx

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm still testing things, so I might change some things, but I want to support the new device_config_file_consistency test. It uses the DeviceConfigFile.hpp, and that file includes DeviceConfigFile.inc, which is generated by tablegen. On CI from what I understand the e2e tests are invoked by using the packed install files from the build step and only runs CMake on the sycl/test-e2e subfolder. So since we probably don't want to build tablegen and invoke other LLVM cmake files when running the e2e tests, I install the DeviceConfigFile.inc it in the build step to pass it to the e2e tests. Also note that this test must be an e2e test as it queries the device it is running the test on, so it can't be moved to sycl/test.

Copy link
Contributor

@sarnex sarnex Dec 20, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

sorry so what tools/files are required to generate that hpp file? Is it just llvm-tablegen and DeviceConfigFile.inc?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yea just tablegen and DeviceConfigFile.td are needed for the hpp file

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also just for some more background I was aiming so that DeviceConfigFile.hpp is not installed by default because outside of testing, this file is not needed for a SYCL distribution, it is only used in the compiler.

@jzc jzc temporarily deployed to WindowsCILock December 19, 2024 18:48 — with GitHub Actions Inactive
@jzc jzc temporarily deployed to WindowsCILock December 19, 2024 18:51 — with GitHub Actions Inactive
@jzc jzc temporarily deployed to WindowsCILock December 19, 2024 20:08 — with GitHub Actions Inactive
@jzc jzc temporarily deployed to WindowsCILock December 19, 2024 20:23 — with GitHub Actions Inactive
maarquitos14
maarquitos14 reviewed Dec 20, 2024
View reviewed changes
@jzc jzc temporarily deployed to WindowsCILock December 23, 2024 17:39 — with GitHub Actions Inactive
maarquitos14
maarquitos14 reviewed Jun 5, 2025
View reviewed changes
Copy link
Contributor

@maarquitos14 maarquitos14 left a comment

Choose a reason for hiding this comment

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

A few nits, but otherwise looks quite good.

@maarquitos14
Copy link
Contributor

Note also that I unresolved a few conversations about punctuation because I don't see them resolved in the code.

maarquitos14
maarquitos14 approved these changes Jun 5, 2025
View reviewed changes
Copy link
Contributor

@maarquitos14 maarquitos14 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 one more nit.

@KornevNikita
Copy link
Contributor

CI:
error: nvidia_gpu_sm_86 does not support aspect ext_intel_max_mem_bandwidth but it is declared in the device config file
So I removed ext_intel_max_mem_bandwidth from CudaMinAspects. I'm not familiar with DeviceConfigFile, is it the right move or I should update exactly the sm_86 list?

@KornevNikita
Copy link
Contributor

@AlexeySachkov if you don't have any further objections - could you please merge?

@AlexeySachkov
Copy link
Contributor

@AlexeySachkov if you don't have any further objections - could you please merge?

We need an approval from @sarnex as a member of @intel/dpcpp-devops-reviewers group

@AlexeySachkov
Copy link
Contributor

CI: error: nvidia_gpu_sm_86 does not support aspect ext_intel_max_mem_bandwidth but it is declared in the device config file So I removed ext_intel_max_mem_bandwidth from CudaMinAspects. I'm not familiar with DeviceConfigFile, is it the right move or I should update exactly the sm_86 list?

I would tag @frasercrmck here, we need someone familiar with CUDA.

@KornevNikita
Copy link
Contributor

CI: error: nvidia_gpu_sm_86 does not support aspect ext_intel_max_mem_bandwidth but it is declared in the device config file So I removed ext_intel_max_mem_bandwidth from CudaMinAspects. I'm not familiar with DeviceConfigFile, is it the right move or I should update exactly the sm_86 list?

I would tag @frasercrmck here, we need someone familiar with CUDA.

The same thing for hip - e098145

sarnex
sarnex approved these changes Jun 11, 2025
View reviewed changes
Copy link
Contributor

@sarnex sarnex left a comment

Choose a reason for hiding this comment

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

configure.py lgtm

@sarnex sarnex merged commit 335b7a0 into intel:sycl Jun 11, 2025
46 of 49 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@KornevNikita KornevNikita KornevNikita left review comments

@AlexeySachkov AlexeySachkov AlexeySachkov approved these changes

@sarnex sarnex sarnex approved these changes

@maarquitos14 maarquitos14 maarquitos14 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@jzc @KornevNikita @maarquitos14 @AlexeySachkov @sarnex