- Notifications
You must be signed in to change notification settings - Fork792
[SYCL][E2E] FixDeviceLib/assert-windows.cpp run-time errors#17493
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
ayylol commentedMar 17, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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.
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 tostderr and here we checkstdout instead for it. With the changes im specifically checkingstderr 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.
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 theurProgramLink 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.
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.
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.
So I am not opposed to removing that check from this e2e test.
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.
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.
b1d9368 intointel:syclUh oh!
There was an error while loading.Please reload this page.
Cherry-pick commits that reached the internal branch between intel/llvmcutoff and release branch pulldown.Patches included:[SYCL][E2E] Fix DeviceLib/assert-windows.cpp run-time errors (#17493)Patch-by: David Garcia Orozco <david.garcia.orozco@intel.com>---[SYCL] Fix memory leak. (#17632)According tohttps://github.com/llvm/llvm-project/blob/main/llvm/unittests/Demangle/OutputBufferTest.cpp,`OutputBuffer` has to be manually freed.Patch-by: Marcos Maronas <marcos.maronas@intel.com>---[SYCL][E2E][Matrix] Cleanup of xfail and unsupported (#17688)Patch-by: Dounia Khaldi <dounia.khaldi@intel.com>---[SYCL][E2E][Matrix] Restrict XFAIL in some tests to DG2 (#17639)XPASSing in[nightly](https://github.com/intel/llvm/actions/runs/14050841978) butfailing in pre/postcommit, make them XFAIL only where they actuallyfail.Patch-by: Nick Sarnie <nick.sarnie@intel.com>---[SYCL][ESIMD] Move RAII deleter init after buffer allocation. (#17706)The buffer seems to be allocated in the call to`NameNode->print(NameBuf)`, so we need to move the RAII deleter afterthat call, otherwise the buffer keeps leaking because when the RAIIdeleter is created, there's no buffer yet.Patch-by: Marcos Maronas <marcos.maronas@intel.com>---[SYCL] Add spill_memory_size unittest (#17657)This adds a unittest for the kernel queries extension according to#17593.Patch-by: Petr Kurapov <petr.a.kurapov@intel.com>---[SYCL][Bindless][E2E] Add 1D host USM tests (#17374)Adds 1D Host USM backed image tests.This patch also fixes device aspect queries for 1D & 2D USM backed imagesampling support.Patch-by: przemektmalon <przemek.malon@codeplay.com>
No description provided.