Skip to content

[SYCL] piextQueueCreateWithNativeHandle should not be followed by a piQueueRetain #4988

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

Merged
merged 5 commits into from
Dec 1, 2021

Conversation

smaslov-intel
Copy link
Contributor

Signed-off-by: Sergey V Maslov [email protected]

@smaslov-intel smaslov-intel requested a review from a team as a code owner November 18, 2021 20:41
Signed-off-by: Sergey V Maslov <[email protected]>
alexbatashev
alexbatashev previously approved these changes Nov 19, 2021
Copy link
Contributor

@romanovvlad romanovvlad left a comment

Choose a reason for hiding this comment

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

Could you please add a unit test to sycl/unittests/?

@smaslov-intel
Copy link
Contributor Author

The fix for the failing test in precommit comes in intel/llvm-test-suite#577

@smaslov-intel
Copy link
Contributor Author

Could you please add a unit test to sycl/unittests/?

I don't know how to write a good unit test for this. The issue was appearing as a "leak" of a queue essentially, since it's reference counter never got to 0 in this case. We have special testing (in process of being enabled by default) design to test imbalance between create/release calls with ZE_DEBUG=4

@romanovvlad
Copy link
Contributor

Could you please add a unit test to sycl/unittests/?

I don't know how to write a good unit test for this. The issue was appearing as a "leak" of a queue essentially, since it's reference counter never got to 0 in this case. We have special testing (in process of being enabled by default) design to test imbalance between create/release calls with ZE_DEBUG=4

We can check that for a specific code(make_queue call) there are expected number of calls to piQueueRetain by mocking this API like in this test https://github.com/intel/llvm/blob/sycl/sycl/unittests/queue/Wait.cpp .

Signed-off-by: Sergey V Maslov <[email protected]>
@smaslov-intel
Copy link
Contributor Author

Could you please add a unit test to sycl/unittests/?

I don't know how to write a good unit test for this. The issue was appearing as a "leak" of a queue essentially, since it's reference counter never got to 0 in this case. We have special testing (in process of being enabled by default) design to test imbalance between create/release calls with ZE_DEBUG=4

We can check that for a specific code(make_queue call) there are expected number of calls to piQueueRetain by mocking this API like in this test https://github.com/intel/llvm/blob/sycl/sycl/unittests/queue/Wait.cpp .

@romanovvlad : please check

Signed-off-by: Sergey V Maslov <[email protected]>
@smaslov-intel
Copy link
Contributor Author

Just in case, the "Plugin/interop-level-zero.cpp" fail is expected with a fix coming in intel/llvm-test-suite#577

Copy link
Contributor

@romanovvlad romanovvlad left a comment

Choose a reason for hiding this comment

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

Thanks, a couple of minor comments.

@romanovvlad
Copy link
Contributor

Just in case, the "Plugin/interop-level-zero.cpp" fail is expected with a fix coming in intel/llvm-test-suite#577

Should this patches be committed simultaneously? Do they depend on each other?

@smaslov-intel
Copy link
Contributor Author

Just in case, the "Plugin/interop-level-zero.cpp" fail is expected with a fix coming in intel/llvm-test-suite#577

Should this patches be committed simultaneously? Do they depend on each other?

The llvm-test-suite change can be committed first, the test still passes with that change. Now CI reports unrelated CUDA failures there.

Signed-off-by: Sergey V Maslov <[email protected]>
@smaslov-intel
Copy link
Contributor Author

@romanovvlad : please merge this

@romanovvlad romanovvlad merged commit 299f506 into intel:sycl Dec 1, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants