[SYCL] Fix KernelFusion/abort_fusion.cpp in "--param ze_debug=-1" mode #9077
Conversation
aelovikov-intel
requested review from
victor-eds,
Naghasan and
sommerlukas
as code owners
| // CHECK-NEXT: Cannot fuse kernels with different offsets or local sizes | ||
| // CHECK-NEXT: COMPUTATION OK | ||
| // CHECK: COMPUTATION OK |
There was a problem hiding this comment.
Do I assume correctly that this change is motivated by additional output with ze_debug in between the output expected by the test?
|
|
||
| return 0; | ||
| return num_errors == 0 ? 0 : 1; |
There was a problem hiding this comment.
Why is this change needed? Test will already fail due to missing COMPUTATION OK if there's an error.
And if a non-zero return code is desirable, should we use an assert for numErrors instead, so we can provide a message in addition to the return code? In that case, the assert could live in performFusion and less changes would be required as part of this PR.
There was a problem hiding this comment.
I'm fine with either non-zero exit code or assert. However, using FileCheck for that purpose is unnatural and an overkill, IMO. Other tests are doing just fine using just the exit code.
Also, checking exit code allows something like
while ./a.out >/dev/null 2>&1 ; do true ; done
when working with flaky failures.
There was a problem hiding this comment.
I agree, using FileCheck to check for computation results is overkill, therefore, for most kernel fusion tests, we use asserts/exit code to detect failure.
This specific test case however uses FileCheck for a specific reason, documented at the beginning of the file:
Also check that warnings are printed when SYCL_RT_WARNING_LEVEL=1.
If you prefer to also have a non-zero exit code, assert could be used and I'm fine with replacing CHECK-NEXT with CHECK to accommodate additional output with ze_debug. The remaining changes to the test-case are not really necessary IMHO.
There was a problem hiding this comment.
Do you want me to assert inside test or main? We currently only have one call to test but if you plan on changing that then asserting in main provides more output in case of early failure.
There was a problem hiding this comment.
Asserting in performFusion should be enough for now.
|
@intel/llvm-gatekeepers , this PR is ready. |
No description provided.