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

[OpenCL] Make extension function lookup return unusupported error #1448

Merged

Conversation

steffenlarsen
Copy link
Contributor

This commit changes the error code of failing to look up extension pointers in OpenCL return UR_RESULT_ERROR_UNSUPPORTED_FEATURE instead of UR_RESULT_ERROR_INVALID_VALUE. Likewise, some of the places this lookup was used, error would return UR_RESULT_ERROR_INVALID_OPERATION. The motivation behind this change is to make the error codes for cases where extensions are not supported for the OpenCL target clearer.

This commit changes the error code of failing to look up extension
pointers in OpenCL return UR_RESULT_ERROR_UNSUPPORTED_FEATURE instead
of UR_RESULT_ERROR_INVALID_VALUE. Likewise, some of the places this
lookup was used, error would return UR_RESULT_ERROR_INVALID_OPERATION.
The motivation behind this change is to make the error codes for cases
where extensions are not supported for the OpenCL target clearer.

Signed-off-by: Larsen, Steffen <[email protected]>
@steffenlarsen steffenlarsen requested review from a team as code owners March 15, 2024 11:09
@codecov-commenter
Copy link

codecov-commenter commented Mar 15, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 12.43%. Comparing base (78ef1ca) to head (a4604d9).
Report is 188 commits behind head on main.

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

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1448      +/-   ##
==========================================
- Coverage   14.82%   12.43%   -2.40%     
==========================================
  Files         250      241       -9     
  Lines       36220    36242      +22     
  Branches     4094     4111      +17     
==========================================
- Hits         5369     4506     -863     
- Misses      30800    31732     +932     
+ Partials       51        4      -47     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@aarongreig
Copy link
Contributor

relates to #1362

steffenlarsen added a commit to steffenlarsen/llvm that referenced this pull request Mar 15, 2024
This commit adds a new error code to PI as a mapping from
UR_RESULT_ERROR_FEATURE_UNSUPPORTED.

This relates to oneapi-src/unified-runtime#1448.

Signed-off-by: Larsen, Steffen <[email protected]>
Signed-off-by: Larsen, Steffen <[email protected]>
@steffenlarsen
Copy link
Contributor Author

relates to #1362

Interesting! This was an ambiguity we ran into when debugging an issue with an unexpected device being picked up in DPC++. I have made a patch to also make a mapping for the unsupported error in intel/llvm as well: intel/llvm#13036

The hope is that this will make it easier to differentiate when the backend is unhappy with a call and when it is simply just not supporting it.

Signed-off-by: Larsen, Steffen <[email protected]>
steffenlarsen added a commit to intel/llvm that referenced this pull request Mar 20, 2024
This commit adds a new error code to PI as a mapping from
UR_RESULT_ERROR_FEATURE_UNSUPPORTED.

This relates to oneapi-src/unified-runtime#1448.

Signed-off-by: Larsen, Steffen <[email protected]>
@steffenlarsen
Copy link
Contributor Author

@kbenzie - I think this is ready.

@aarongreig aarongreig added the ready to merge Added to PR's which are ready to merge label Apr 2, 2024
@kbenzie kbenzie added the opencl OpenCL adapter specific issues label Apr 3, 2024
@kbenzie kbenzie merged commit 3609afc into oneapi-src:main Apr 4, 2024
51 checks passed
@hvdijk
Copy link

hvdijk commented Apr 8, 2024

Hi, there were some checks for functions that were followed by a if (FuncPtr) check and properly handled the optional function not being implemented. It looks like they now return an error for that, is that right? If I am reading this right (and I may well not, please correct me if so) this change basically means that OpenCL implementations other than Intel's own can no longer be used. I'm seeing this with OCK where we now get e.g. when running SYCL-CTS

---> piMemBufferCreate(
	<unknown> : 0xa204c80
	<unknown> : 9
	<unknown> : 40
	<unknown> : 0x7ffff86c0910
	<unknown> : 0x7ffff86bf5a0
	<unknown> : 0
) ---> 	pi_result : -995
	[out]void * : 0x7ffff86c0910
	[out]pi_mem * : 0x7ffff86bf5a0[ 0 ... ]


~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
The SYCL 2020 Conformance Test Suite is a Catch2 v3.2.1 host application.
Run with -? for options

-------------------------------------------------------------------------------
requisite for the entire underlying buffer for sycl::accessor 
-------------------------------------------------------------------------------
/builds/ComputeAorta/ci-github/SYCL-CTS/tests/accessor/accessor_requisite_entire_buffer.cpp:32
...............................................................................

/builds/ComputeAorta/ci-github/SYCL-CTS/tests/accessor/accessor_requisite_entire_buffer.cpp:32: FAILED:
due to unexpected exception with message:
  SYCL exception
  with category name: 'sycl'
  with code: 'runtime'
  with code raw value: '1'
  with code message: 'SYCL Error'
  with what: 'Native API failed. Native API returns: -995 (The plugin or device
  does not support the called function) -995 (The plugin or device does not
  support the called function)'

===============================================================================
test cases: 1 | 1 failed
assertions: 1 | 1 failed

@coldav
Copy link
Contributor

coldav commented Apr 8, 2024

note that OCK was passing (fast) SYCL CTS before this point, so it suggests that perhaps we have made something optional an error.

steffenlarsen added a commit to steffenlarsen/unified-runtime that referenced this pull request Apr 9, 2024
This commit reverts the urMemBufferCreate changes for returning function
lookup error in oneapi-src#1448
as the function had a fallback path which is no longer taken.

Signed-off-by: Larsen, Steffen <[email protected]>
@steffenlarsen
Copy link
Contributor Author

Thank you for letting me know, @hvdijk & @coldav ! You are absolutely right, the changes to {{urMemBufferCreate}} were not right. They can be safely reverted, as done in #1496.

kbenzie added a commit to kbenzie/llvm that referenced this pull request Apr 9, 2024
@kbenzie kbenzie added the v0.9.x Include in the v0.9.x release label Apr 17, 2024
@kbenzie kbenzie mentioned this pull request Apr 17, 2024
19 tasks
kbenzie added a commit that referenced this pull request Apr 17, 2024
…l_unsupported

[OpenCL] Make extension function lookup return unusupported error
kbenzie pushed a commit to kbenzie/llvm that referenced this pull request Apr 17, 2024
This commit adds a new error code to PI as a mapping from
UR_RESULT_ERROR_FEATURE_UNSUPPORTED.

This relates to oneapi-src/unified-runtime#1448.

Signed-off-by: Larsen, Steffen <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
opencl OpenCL adapter specific issues ready to merge Added to PR's which are ready to merge v0.9.x Include in the v0.9.x release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants