Skip to content

Improvements to align CTS and Spec for Context #2112

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 2 commits into from
Nov 11, 2024
Merged

Improvements to align CTS and Spec for Context #2112

merged 2 commits into from
Nov 11, 2024

Conversation

@martygrant
Copy link
Contributor

@martygrant martygrant commented Sep 20, 2024 •
edited
Loading

intel/llvm#15506

For urContextGetInfo some adapters were returning UR_RESULT_ERROR_INVALID_ENUMERATION when they did not support the enumeration yet, instead of UR_RESULT_ERROR_UNSUPPORTED_ENUMERATION. I've updated this, and modified the CTS test for it to pass when the unsupported enum is received.

Added another CTS test for urContextCreate to test for the UR_RESULT_ERROR_INVALID_ENUMERATION return scenario.

@martygrant martygrant requested review from a team as code owners September 20, 2024 16:02
@github-actions github-actions bot added conformance Conformance test suite issues. level-zero L0 adapter specific issues cuda CUDA adapter specific issues hip HIP adapter specific issues native-cpu Native CPU adapter specific issues labels Sep 20, 2024
@MartinWehking
Copy link

MartinWehking commented Sep 23, 2024

Hi @martygrant ,
just a nit, but I do find the PR description a bit confusing (and the formatting might be a bit off).
You basically changedurContextGetInfo for all adapters to return UR_RESULT_ERROR_UNSUPPORTED_ENUMERATION
as common error (i.e. for the same properties).
You added test cases to check for this specifically, right?

@martygrant martygrant force-pushed the martin/context-cts-spec-gap branch from b3cf34b to 2ae73ff Compare September 23, 2024 16:47
@martygrant
Copy link
Contributor Author

martygrant commented Sep 23, 2024 •
edited
Loading

hey @MartinWehking yeah that is mostly correct. So UR_RESULT_ERROR_UNSUPPORTED_ENUMERATION should just be returned when it is a correct property passed in, but the adapter hasn't implemented it yet. I modified urContextGetInfoTestWithInfoParam so that it will pass if UR_RESULT_ERROR_UNSUPPORTED_ENUMERATION is returned as that's a valid scenario

Apologies the PR description is probably less obvious for someone not on the UR team, I should maybe rewrite it!

@MartinWehking
Copy link

MartinWehking commented Sep 24, 2024

@martygrant
Ok, I see, in that case: LGTM.
Thanks for the clarification.
Updating the PR message would be great.

@martygrant martygrant mentioned this pull request Sep 25, 2024
Merged
@martygrant martygrant force-pushed the martin/context-cts-spec-gap branch 5 times, most recently from 166b0a1 to 652993e Compare September 27, 2024 12:04
RossBrunton
RossBrunton reviewed Sep 30, 2024
View reviewed changes
@martygrant martygrant force-pushed the martin/context-cts-spec-gap branch 6 times, most recently from 1174159 to cc698e7 Compare October 3, 2024 08:14
@martygrant martygrant force-pushed the martin/context-cts-spec-gap branch from cc698e7 to a580eaa Compare October 3, 2024 15:04
PietroGhg
PietroGhg approved these changes Oct 7, 2024
View reviewed changes
Copy link
Contributor

@PietroGhg PietroGhg left a comment

Choose a reason for hiding this comment

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

Native CPU LGTM, thank you

@martygrant
Copy link
Contributor Author

martygrant commented Oct 7, 2024

@oneapi-src/unified-runtime-level-zero-write looking for a review on this PR please

@martygrant martygrant force-pushed the martin/context-cts-spec-gap branch from a580eaa to f3889a4 Compare October 21, 2024 16:35
@martygrant
Copy link
Contributor Author

martygrant commented Oct 31, 2024

@oneapi-src/unified-runtime-level-zero-write looking for a review on this PR please

pbalcer
pbalcer approved these changes Oct 31, 2024
View reviewed changes
Copy link
Contributor

@pbalcer pbalcer 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 a question on the match file changes.

@martygrant
Improvements to align CTS and Spec for Context:
61bc0bd 
- Add test for urContextCreate returning UR_RESULT_ERROR_INVALID_ENUMERATION
- Added testing in urContextGetInfo for the atomic memory enums, checking against the type mask
or UR_RESULT_ERROR_UNSUPPORTED_ENUM (as some adapters don't support this currently)
@martygrant
Remove urContextCreateWithNativeHandleTest.SuccessWithUnOwnedNativeHa…
2a08189 
…ndle from L0 match file
@martygrant martygrant force-pushed the martin/context-cts-spec-gap branch from 0ad7ea4 to 2a08189 Compare November 1, 2024 17:28
@martygrant martygrant added the ready to merge Added to PR's which are ready to merge label Nov 4, 2024
@callumfare callumfare merged commit 2eae687 into oneapi-src:main Nov 11, 2024
74 of 77 checks passed
martygrant added a commit to intel/llvm that referenced this pull request Nov 11, 2024
@martygrant @callumfare
[UR] Bump tag to 2eae687 for UR#2112 (#15506)
6d1f4fc 
For oneapi-src/unified-runtime#2112

---------

Co-authored-by: Callum Fare <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@pbalcer pbalcer pbalcer approved these changes

+4 more reviewers

@RossBrunton RossBrunton RossBrunton left review comments

@kbenzie kbenzie kbenzie approved these changes

@MartinWehking MartinWehking MartinWehking approved these changes

@PietroGhg PietroGhg PietroGhg approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

conformance Conformance test suite issues. cuda CUDA adapter specific issues hip HIP adapter specific issues level-zero L0 adapter specific issues native-cpu Native CPU adapter specific issues ready to merge Added to PR's which are ready to merge

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@martygrant @MartinWehking @RossBrunton @kbenzie @pbalcer @PietroGhg @callumfare