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

[SYCL][E2E] Fix DeviceLib/assert-windows.cpp run-time errors #17493

Merged
merged 4 commits into from
Mar 20, 2025
+6 −13
Merged

[SYCL][E2E] Fix DeviceLib/assert-windows.cpp run-time errors #17493

Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
472c8b5
Fix DeviceLib/assert-windows.cpp
ayylol Mar 17, 2025
f8d6d74
Remove cpu requirement from test
ayylol Mar 19, 2025
eac561a
Merge branch 'sycl' into assertwin
ayylol Mar 20, 2025
3e1f3a9
Change XFAIL tracker issue number
ayylol Mar 20, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 6 additions & 13 deletions sycl/test-e2e/DeviceLib/assert-windows.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,22 +1,15 @@
// REQUIRES: cpu,windows
// REQUIRES: windows
// XFAIL: opencl && gpu
// XFAIL-TRACKER: https://github.com/intel/llvm/issues/11364
//
// RUN: %{build} -o %t.out
// RUN: %{build} -DSYCL_FALLBACK_ASSERT=1 -o %t.out
//
// MSVC implementation of assert does not call an unreachable built-in, so the
// program doesn't terminate when fallback is used.
//
// FIXME: SPIR-V Unreachable should be called from the fallback
// explicitly. Since the test is going to crash, we'll have to follow a similar
// approach as on Linux - call the test in a subprocess.
//
// RUN: env SYCL_UR_TRACE=2 SYCL_DEVICELIB_INHIBIT_NATIVE=1 CL_CONFIG_USE_VECTORIZER=False %{run} %t.out | FileCheck %s --check-prefix=CHECK-FALLBACK
Copy link
Contributor Author

@ayylol ayylol Mar 17, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I wasn't too sure what the correct fix for this line would be. Looking at the UR Trace when we crash, vs not crash the urProgramLink function never appears in the trace. To me it seemed suspicious given the git history, since this line has been changed multiple times since the test had been XFAILed, so its never been confirmed to pass with the changes that have happened since.

Copy link
Contributor

@againull againull Mar 17, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I believe urProgramLink call is supposed to be emitted in case when a backend doesn't support assert natively and we have to link the fallback assert device library at runtime.
I believe cpu backend supports assert natively, so I don't understand why this check has been added.
I belive such verification needs to be done in our unit tests, i.e. by mocking UR to report that backend doesn't support native assert and then verify that urProgramLink is called in this case.

Copy link
Contributor

Choose a reason for hiding this comment

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

So I am not opposed to removing that check from this e2e test.

Copy link
Contributor

Choose a reason for hiding this comment

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

Actually another question is why this test is limited to cpu. I believe it has to be enabled for gpu too or marked explicitly as failing on gpu if there is a problem.

// RUN: env SHOULD_CRASH=1 SYCL_DEVICELIB_INHIBIT_NATIVE=1 CL_CONFIG_USE_VECTORIZER=False %{run} %t.out | FileCheck %s --check-prefix=CHECK-MESSAGE
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This line failed because the error message is sent to stderr and here we check stdout instead for it. With the changes im specifically checking stderr instead

// RUN: not env SHOULD_CRASH=1 SYCL_DEVICELIB_INHIBIT_NATIVE=1 CL_CONFIG_USE_VECTORIZER=False \
// RUN: %{run} %t.out 2>&1 >/dev/null | FileCheck %s --check-prefix=CHECK-MESSAGE
//
// CHECK-MESSAGE: {{.*}}assert-windows.cpp:{{[0-9]+}}: (null): global id:
// [{{[0-3]}},0,0], local id: [{{[0-3]}},0,0] Assertion `accessorC[wiID] == 0 &&
// "Invalid value"` failed.
//
// CHECK-FALLBACK: <--- urProgramLink

#include "../helpers.hpp"
#include <array>
Expand Down