Skip to content

[SYCL][E2E] Fix preview flag detection for CUDA and HIP #11929

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
Nov 28, 2023

Conversation

steffenlarsen
Copy link
Contributor

The detection of support for -fpreview-breaking-changes was failing in E2E unintentionally. This commit fixes this detection.

The detection of support for -fpreview-breaking-changes was failing in
E2E unintentionally. This commit fixes this detection.

Signed-off-by: Larsen, Steffen <[email protected]>
@steffenlarsen steffenlarsen requested a review from a team as a code owner November 17, 2023 12:57
@steffenlarsen
Copy link
Contributor Author

steffenlarsen commented Nov 21, 2023

@npmiller - It looks like something might be broken for vectors on HIP and CUDA hidden behind broken testing of the preview flag. Could you help investigate?

Note: Since it only affects vec and that math builtins behind the flags were just cleanly moved, I suspect it has more to do with #11697 than the builtins. Tag @cperkinsintel for awareness.

steffenlarsen added a commit to steffenlarsen/llvm that referenced this pull request Nov 22, 2023
The preview flag testing for E2E testing is currently broken.
intel#11929 attempts to fix it but has
shown that some breakages in CUDA and HIP have been hidden behind the
broken testing. To allow for proper testing while these failures are
being investigated, this PR implements the fixes from
intel#11929 with the caveat that they do
not apply when HIP and CUDA devices are part of the testing. This is
a temporary solution and does *not* replace
intel#11929.

Signed-off-by: Larsen, Steffen <[email protected]>
@konradkusiak97
Copy link
Contributor

konradkusiak97 commented Nov 23, 2023

Hi @steffenlarsen, it seems like the second RUN lit command (the one with -fpreview-breaking-changes flag) in all the failing tests, doesn't include -fsycl-targets=... and that causes creating an invalid binary.

Here is an example fix for vec_common.cpp:

--- a/sycl/test-e2e/Basic/built-ins/vec_common.cpp
+++ b/sycl/test-e2e/Basic/built-ins/vec_common.cpp
@@ -1,6 +1,6 @@
 // RUN: %{build} -o %t.out
 // RUN: %{run} %t.out
-// RUN: %if preview-breaking-changes-supported %{ %clangxx -fsycl -fpreview-breaking-changes %s -o %t2.out %}
+// RUN: %if preview-breaking-changes-supported %{ %{build} -fpreview-breaking-changes -o %t2.out %}
 // RUN: %if preview-breaking-changes-supported %{ %{run} %t2.out %}
 
 #ifdef _WIN32

I'd be happy to include here the whole patch but Github doesn't allow me to

steffenlarsen added a commit that referenced this pull request Nov 23, 2023
…#11978)

The preview flag testing for E2E testing is currently broken.
#11929 attempts to fix it but has
shown that some breakages in CUDA and HIP have been hidden behind the
broken testing. To allow for proper testing while these failures are
being investigated, this PR implements the fixes from
#11929 with the caveat that they do
not apply when HIP and CUDA devices are part of the testing. This is a
temporary solution and does *not* replace
#11929.

---------

Signed-off-by: Larsen, Steffen <[email protected]>
Co-authored-by: Chris Perkins <[email protected]>
@steffenlarsen steffenlarsen changed the title [SYCL][E2E] Fix preview flag detection [SYCL][E2E] Fix preview flag detection for CUDA and HIP Nov 23, 2023
Signed-off-by: Larsen, Steffen <[email protected]>
@steffenlarsen
Copy link
Contributor Author

Hi @steffenlarsen, it seems like the second RUN lit command (the one with -fpreview-breaking-changes flag) in all the failing tests, doesn't include -fsycl-targets=... and that causes creating an invalid binary.

Here is an example fix for vec_common.cpp:

--- a/sycl/test-e2e/Basic/built-ins/vec_common.cpp
+++ b/sycl/test-e2e/Basic/built-ins/vec_common.cpp
@@ -1,6 +1,6 @@
 // RUN: %{build} -o %t.out
 // RUN: %{run} %t.out
-// RUN: %if preview-breaking-changes-supported %{ %clangxx -fsycl -fpreview-breaking-changes %s -o %t2.out %}
+// RUN: %if preview-breaking-changes-supported %{ %{build} -fpreview-breaking-changes -o %t2.out %}
 // RUN: %if preview-breaking-changes-supported %{ %{run} %t2.out %}
 
 #ifdef _WIN32

I'd be happy to include here the whole patch but Github doesn't allow me to

Good catch, @konradkusiak97 ! That would definitely do it. I've pushed fixes to all the cases I could find with this. Let's hope that's the problem. Thanks a ton!

Signed-off-by: Larsen, Steffen <[email protected]>
@steffenlarsen
Copy link
Contributor Author

Looks like that was exactly the issue. Thanks again, @konradkusiak97 !

steffenlarsen added a commit that referenced this pull request Nov 23, 2023
This commit refactors the way we define source programs used to detect
the availability of libraries in SYCL e2e LIT testing.

Suggestion by @ldrumm in
#11929 (comment).

---------

Signed-off-by: Larsen, Steffen <[email protected]>
Co-authored-by: Dmitry Vodopyanov <[email protected]>
@JackAKirk JackAKirk mentioned this pull request Jan 8, 2024
7 tasks
steffenlarsen added a commit to steffenlarsen/llvm that referenced this pull request Jan 8, 2024
The detection of support for -fpreview-breaking-changes was failing in
E2E unintentionally. This commit fixes this detection.

---------

Signed-off-by: Larsen, Steffen <[email protected]>
(cherry picked from commit bc30bef)
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.

5 participants