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.

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

Improvements to align CTS and Spec for Context #2112

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Improvements to align CTS and Spec for Context #2112

wants to merge 1 commit into from
+77 −38

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
Contributor

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
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
Contributor

@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
Draft
@martygrant
Improvements to align CTS and Spec for Context:
ea5afa9 
- 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)
	- Updated some adapter code to include these enums and return the correct error code
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MartinWehking MartinWehking MartinWehking approved these changes

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
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@martygrant @MartinWehking