unified-runtime icon indicating copy to clipboard operation
unified-runtime copied to clipboard

[Spec Constants] Improved handling of invalid spec. constants

Open RossBrunton opened this issue 1 year ago • 8 comments

Two main changes to how Kernel/ProgramSetSpecializationConstants are handled:

  • They may now output either INVALID_VALUE or the new INVALID_SPEC_ID when the provided list is invalid.
  • The OpenCL and level 0 adapters now respond to UR_DEVICE_INFO_KERNEL_SET_SPECIALIZATION_CONSTANTS with false rather than erroring out. This fixes some tests that were incorrectly not being skipped.
  • urKernelSetSpecializationConstants now "implemented" (as a function that returns UNSUPPORTED_FEATURE for opencl and cuda.

RossBrunton avatar Mar 22 '24 17:03 RossBrunton

Codecov Report

Attention: Patch coverage is 0% with 29 lines in your changes are missing coverage. Please review.

Project coverage is 12.42%. Comparing base (78ef1ca) to head (7bb6c27). Report is 199 commits behind head on main.

Files Patch % Lines
...ance/kernel/urKernelSetSpecializationConstants.cpp 0.00% 13 Missing :warning:
...ce/program/urProgramSetSpecializationConstants.cpp 0.00% 13 Missing :warning:
include/ur_print.hpp 0.00% 3 Missing :warning:

:exclamation: Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1469      +/-   ##
==========================================
- Coverage   14.82%   12.42%   -2.41%     
==========================================
  Files         250      241       -9     
  Lines       36220    36271      +51     
  Branches     4094     4111      +17     
==========================================
- Hits         5369     4506     -863     
- Misses      30800    31761     +961     
+ Partials       51        4      -47     

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

codecov-commenter avatar Mar 22 '24 17:03 codecov-commenter

@jchlanda is our spec constants expert. Jakub, please take over this review

ldrumm avatar Mar 29 '24 15:03 ldrumm

could you fix the conflicts @RossBrunton ?

kbenzie avatar Apr 29 '24 15:04 kbenzie

@RossBrunton I think the HIP match files need updated on this.

kbenzie avatar May 13 '24 11:05 kbenzie

@RossBrunton I think the HIP match files need updated on this.

Forgot you were off. I would make the changes to this PR myself but the checkbox to allow project maintainers to make changes appears to have been disabled.

kbenzie avatar May 14 '24 08:05 kbenzie

Can someone from @oneapi-src/unified-runtime-level-zero-write have a quick look at this? The changes for level_zero are reporting that it doesn't support UR_DEVICE_INFO_KERNEL_SET_SPECIALIZATION_CONSTANTS (which it doesn't yet).

RossBrunton avatar May 23 '24 10:05 RossBrunton

@RossBrunton Please rebase this when possible, I removed the ready to merge label and please readd it when rebased.

omarahmed1111 avatar Sep 18 '24 07:09 omarahmed1111

@RossBrunton Please rebase this when possible, I removed the ready to merge label and please readd it when rebased.

Updated. Sorry for the delay.

EDIT: Never mind...

EDIT 2: And it's green again and should be ready to merge.

RossBrunton avatar Oct 01 '24 13:10 RossBrunton