Skip to content

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