Skip to content

Conversation

@RossBrunton
Copy link
Contributor

@RossBrunton RossBrunton commented Jan 9, 2025

THis is similar to the existing
DEVICE_INFO_KERNEL_SET_SPECIALIZATION_CONSTANTS device info, only
it applies to programs rather than kernels. All of the adapters should
be updated to report it correctly, and tests have been added.

@github-actions github-actions bot added loader Loader related feature/bug conformance Conformance test suite issues. specification Changes or additions to the specification level-zero L0 adapter specific issues cuda CUDA adapter specific issues hip HIP adapter specific issues opencl OpenCL adapter specific issues native-cpu Native CPU adapter specific issues labels Jan 9, 2025
@RossBrunton RossBrunton force-pushed the ross/specconst branch 3 times, most recently from cf78063 to 2b4c661 Compare January 9, 2025 14:59
@RossBrunton RossBrunton force-pushed the ross/specconst branch 3 times, most recently from a26df8c to 5882d68 Compare January 13, 2025 11:08
@RossBrunton RossBrunton marked this pull request as ready for review January 15, 2025 11:30
@RossBrunton RossBrunton requested review from a team as code owners January 15, 2025 11:30
@RossBrunton RossBrunton requested a review from ldrumm January 15, 2025 11:30
Copy link
Contributor

@frasercrmck frasercrmck left a comment

Choose a reason for hiding this comment

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

CUDA/HIP LGTM

@RossBrunton RossBrunton force-pushed the ross/specconst branch 2 times, most recently from 57687c0 to be7c5e7 Compare January 15, 2025 14:18
@RossBrunton
Copy link
Contributor Author

@oneapi-src/unified-runtime-native-cpu-write @oneapi-src/unified-runtime-level-zero-write Can you quickly look at this? I've just set it so that Native CPU and Level Zero don't advertise support for specialization constants.

@coldav
Copy link

coldav commented Jan 16, 2025

@oneapi-src/unified-runtime-native-cpu-write @oneapi-src/unified-runtime-level-zero-write Can you quickly look at this? I've just set it so that Native CPU and Level Zero don't advertise support for specialization constants.

It might makes sense to get @PietroGhg to review (not sure if he is still in these groups or not) - we will want specialized constants for native cpu though I think.

@kbenzie
Copy link
Contributor

kbenzie commented Jan 16, 2025

we will want specialized constants for native cpu though I think.

It doesn't support them today so it should say so to the user.

@coldav
Copy link

coldav commented Jan 16, 2025

we will want specialized constants for native cpu though I think.

It doesn't support them today so it should say so to the user.

I thought we did, just that we fail tests.

@RossBrunton
Copy link
Contributor Author

we will want specialized constants for native cpu though I think.

It doesn't support them today so it should say so to the user.

I thought we did, just that we fail tests.

Both urKernelSetSpecializationConstants and urProgramSetSpecializationConstants return UR_RESULT_ERROR_UNSUPPORTED_FEATURE in native CPU.

@coldav
Copy link

coldav commented Jan 17, 2025

we will want specialized constants for native cpu though I think.

It doesn't support them today so it should say so to the user.

I thought we did, just that we fail tests.

Both urKernelSetSpecializationConstants and urProgramSetSpecializationConstants return UR_RESULT_ERROR_UNSUPPORTED_FEATURE in native CPU.

okay that's fine, as long as we can easily switch this back on.

This is similar to the existing
`DEVICE_INFO_KERNEL_SET_SPECIALIZATION_CONSTANTS` device info, only
it applies to programs rather than kernels. All of the adapters should
be updated to report it correctly, and tests have been added.
@RossBrunton
Copy link
Contributor Author

@oneapi-src/unified-runtime-level-zero-write I want to get this merged, can you look at it?

@coldav @PietroGhg It should be as simple as implementing it and returning true for this device info. Once that is done, it should "just work" and (properly written) user code will switch over to using spec constants. If you're okay with it, can I get an approval?

@PietroGhg
Copy link
Contributor

we will want specialized constants for native cpu though I think.

It doesn't support them today so it should say so to the user.

I thought we did, just that we fail tests.

Both urKernelSetSpecializationConstants and urProgramSetSpecializationConstants return UR_RESULT_ERROR_UNSUPPORTED_FEATURE in native CPU.

For Native CPU we are going through the "emulated" path for Spec Constants, which means (I may be wrong, it's been a while since I looked into it) that the values for them are set as kernel arguments by the SYCL runtime, so I think that the values for urKernelSetSpecializationConstants and urProgramSetSpecializationConstants won't affect Native CPU, so this change looks fine to me

@RossBrunton RossBrunton merged commit b074893 into oneapi-src:main Jan 21, 2025
73 checks passed
RossBrunton added a commit to RossBrunton/intel-llvm that referenced this pull request Jan 21, 2025
@RossBrunton RossBrunton deleted the ross/specconst branch January 21, 2025 11:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

conformance Conformance test suite issues. cuda CUDA adapter specific issues hip HIP adapter specific issues level-zero L0 adapter specific issues loader Loader related feature/bug native-cpu Native CPU adapter specific issues opencl OpenCL adapter specific issues specification Changes or additions to the specification

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants