-
Notifications
You must be signed in to change notification settings - Fork 779
[SYCL][E2E] Fix -Werror failures in implicitly SPIR only DeviceLib tests #18995
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
base: sycl
Are you sure you want to change the base?
Conversation
The `-fsycl-device-lib-jit-link` only has an effect in SPIR compilation so these tests would fail with `-Werror` for non SPIR targets. This basically the same as intel#18985, but also apply it to tests that have `UNSUPPORTED: target-amd || target-nvidia`. These tests could be enabled for future non-SPIR targets we should not be assuming that `!target-amd && !target-nvidia` implies SPIR.
We have |
Are you asking me or @Maetveis ? |
The author of the text from the PR description quoted in my comment. |
…k` appears Use `%if target-spir` to either conditionally enable the entire run-line (when the run-line only only differs by adding `-fsycl-device-lib-jit-link`), or to conditionally enable the `-fsycl-device-lib-jit-link` flag itself (when there are other differences).
I'd rather not. These tests are not SPIR specific IMO, only |
|
|
||
// RUN: %clangxx -fsycl -fsycl-device-lib-jit-link %{mathflags} %s -o %t.out | ||
// RUN: %if !gpu %{ %{run} %t.out %} | ||
// RUN: %if target-spir %{ %{build} -fsycl-device-lib-jit-link %{mathflags} %s -o %t2.out %} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's a drive-by fix here: %clangxx -fsycl
depends on the default of -fsycl-targets
being spir64
. %{build}
is the same as %clangxx -fsycl %{sycl-target-options %}
which is what we want.
Also the two executables were named the same, so in separate build and run mode the second run line one would override the first, so that would be run twice when starting the test run.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks.
This version looks better.
Hopefully we can fix CI issues still applying the approach with conditional run commands.
Co-authored-by: Alexey Bader <[email protected]>
The expansion of `%{build}` already contains the file name of the test (`%s`), repeating it leads to duplicate symbol errors.
I believe the CI failures are not related to these changes. I see similar failures in other PRs; I opened: #19034 |
The
-fsycl-device-lib-jit-link
only has an effect in SPIR compilation so these tests would fail with-Werror
for non SPIR targets.This basically the same as #18985, but also apply it to tests that have
UNSUPPORTED: target-amd || target-nvidia
. These tests could be enabled for future non-SPIR targets; we should not be assuming that!target-amd && !target-nvidia
implies SPIR.