[SYCL][E2E] Fix DeviceLib/assert-windows.cpp run-time errors
#17493
Conversation
|
For Context: This test had been XFAILed 5 years ago. The XFAIL tracker (#16507) pointed to a more recent build failure, and once that was fixed the XFAIL was removed from the test. However the original reason the test was XFAILed was not resolved, and thus this test failed in internal testing. We dont test windows cpu on github ci so this wasnt caught in pre/post commit. |
| // 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 | ||
| // RUN: env SHOULD_CRASH=1 SYCL_DEVICELIB_INHIBIT_NATIVE=1 CL_CONFIG_USE_VECTORIZER=False %{run} %t.out | FileCheck %s --check-prefix=CHECK-MESSAGE |
There was a problem hiding this comment.
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
| // 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 |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
So I am not opposed to removing that check from this e2e test.
There was a problem hiding this comment.
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.
No description provided.