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

[SYCL][E2E] Fix bindless images tests to run on Level Zero devices #14779

Merged
merged 2 commits into from
Jul 31, 2024
Merged

[SYCL][E2E] Fix bindless images tests to run on Level Zero devices #14779

merged 2 commits into from
Jul 31, 2024

Conversation

nrspruit
Copy link
Contributor

No description provided.

@nrspruit nrspruit requested a review from a team as a code owner July 25, 2024 17:52
@alycm
Copy link
Contributor

alycm commented Jul 25, 2024

I don't know if it important, but the test configuration changes that were previously reverted which more constrained than running on all level zero devices: #14245

@nrspruit
Copy link
Contributor Author

Currently, all the bindless support added to L0 is not being validated because the tests excluded all but CUDA.

ProGTX
ProGTX reviewed Jul 29, 2024
View reviewed changes
sycl/test-e2e/bindless_images/read_1D.cpp

// RUN: %{build} -o %t.out
// RUN: %t.out
// RUN: env NEOReadDebugKeys=1 UseBindlessMode=1 UseExternalAllocatorForSshAndDsh=1 %t.out
Copy link
Contributor

Choose a reason for hiding this comment

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

How come only this test requires these environment variables? Maybe we should add them to the LIT environment for all Bindless tests?

Copy link
Contributor

@wenju-he wenju-he Jul 30, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

these env are required for all tests in this PR for https://github.com/intel/compute-runtime/releases/tag/24.26.30049.6 release

sycl/test-e2e/bindless_images/read_1D.cpp
@@ -1,7 +1,7 @@
// REQUIRES: cuda
// REQUIRES: cuda || (level_zero && gpu-intel-dg2)
Copy link
Contributor

Choose a reason for hiding this comment

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

Doesn't this require a specific driver version? Should that be documented somewhere?

@wenju-he
Copy link
Contributor

This PR could be submitted once 24.26 gpu driver is uplifted in #14838

In addition, array/read_write_unsampled_array.cpp is also passing with 24.26 driver. So this test can also be enabled in this PR.

@nrspruit nrspruit mentioned this pull request Jul 30, 2024
Merged
@nrspruit
Copy link
Contributor Author

@wenju-he , the PR for the gpu uplift was completed, please check to see if there are any other issues such that we have the opportunity to merge.

ProGTX
ProGTX approved these changes Jul 30, 2024
View reviewed changes
Copy link
Contributor

@ProGTX ProGTX left a comment

Choose a reason for hiding this comment

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

Specifying the environment in each test individually is not the preferred solution, but I think it's best we get this in ASAP.

wenju-he
wenju-he approved these changes Jul 31, 2024
View reviewed changes
Copy link
Contributor

@wenju-he wenju-he left a comment •
edited
Loading

Choose a reason for hiding this comment

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

LGTM

My local testing shows 17 out of 18 tests pass and remaining 1 test vulkan_interop/unsampled_images.cpp is unsupported:

UNSUPPORTED: SYCL :: bindless_images/vulkan_interop/unsampled_images.cpp (78 of 95)
No supported devices to run the test on

@wenju-he
Copy link
Contributor

@nrspruit vulkan_interop/unsampled_images.cpp requires import_external_semaphore support, while vulkan_interop/sampled_images.cpp doesn't.
Please double check if PR should enable vulkan_interop/sampled_images.cpp instead.

@wenju-he
Copy link
Contributor

the new commit LGTM

@nrspruit
Copy link
Contributor Author

@intel/llvm-gatekeepers please merge, thank you

@sommerlukas sommerlukas merged commit 1354ff2 into intel:sycl Jul 31, 2024
15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wenju-he wenju-he wenju-he approved these changes

@ProGTX ProGTX ProGTX approved these changes

@stdale-intel stdale-intel Awaiting requested review from stdale-intel

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@nrspruit @alycm @wenju-he @ProGTX @sommerlukas