[SYCL][NFC] Optimize getKernelNamesUsingAssert #5196
Conversation
Now it traverses reversed call graph by BFS algorithm from __devicelib_assert_fail function up to SPIR kernels.
bader
changed the title
| ; Function Attrs: convergent norecurse mustprogress | ||
| define weak_odr dso_local spir_kernel void @"_ZTSZZ4mainENK3$_0clERN2cl4sycl7handlerEE7Kernel8"() local_unnamed_addr #0 { | ||
| call spir_func void @_Z1Gv() | ||
| call spir_func void @_Z1Hv() | ||
| ret void | ||
| } | ||
|
|
||
| ; CHECK-NOT: _ZTSZZ4mainENK3$_0clERN2cl4sycl7handlerEE7Kernel6 |
There was a problem hiding this comment.
Please note that CHECK-NOT is bounded by surrounding CHECK directives. If you want to ensure that some patters is not present in the file at all, then you should use --implicit-check-not
There was a problem hiding this comment.
Unfortunately, today I learnt that implicit-check-not option doesn't work with CHECK-DAG. At least, in a way which I got from documentation.
For example, consider the following inputs:
file:
a
b
c
ll:
; CHECK-DAG: a
; CHECK-DAG: c
If you run FileCheck ll -input-file=./file --implicit-check-not b then you get exit code 0 while I would expect to get non-zero.
There was a problem hiding this comment.
Unfortunately, today I learnt that implicit-check-not option doesn't work with CHECK-DAG. At least, in a way which I got from documentation.
Yeah, I see it too now.
I did that kernel' names print in a sorted order so that it would be easier to check them with simple CHECK:. You might have an approach better than mine.
We shouldn't be doing extra actions like sorting strings just to make our unit-tests pass. Better solution would be to still work on tests: let's just have two separate FileCheck runs on the same input, but with different prefixes (--check-prefix). The first one will check that expected names are present in the result through CHECK-DAG, the second one will only check that unexpected names are not present in the result.
There was a problem hiding this comment.
Thank you! Done that way.
|
/summary:run |
| const Instruction *I = cast<const Instruction>(U); | ||
| const Function *ParentF = I->getFunction(); |
There was a problem hiding this comment.
I'm concerned that there could be other uses of function, which are not instructions, but ConstantExpr. To generate such LLVM IR we can try to cast one function pointer type into another before doing a call. There is an example of such LLVM IR in the translator repo
Another concern is that theoretically, an Instruction can be located in module scope, i.e. outside of a function. Therefore, I->getFunction() will return nullptr. There is an example of such LLVM IR in the translator repo as well. Note: I don't have good idea about how to generate such LLVM IR out of a SYCL app if you want to replicate it yourself.
There was a problem hiding this comment.
Added dyn_cast to CallInst.
There was a problem hiding this comment.
Please note that even though you have fixed a nullptr dereference, functionally we still have an issue, i.e. we can say that there are no assert uses whilst there are through more complex usages of function pointers.
That is probably a minor problem at the moment, because function pointers are not widely used, but we should leave a FIXME here, at least, mentioning the use cases we don't analyze properly.
There was a problem hiding this comment.
Currently, there is a kernel empty_kernel in assert-indirect-with-split-2 which checks that traversing doesn't go in such cases.
Kernel's names in tests are sorted so that order of checking was strictly fixed.
|
I did that kernel' names print in a sorted order so that it would be easier to check them with simple |
|
Two failing tests were fixed about an hour ago in intel/llvm-test-suite#683 and intel/llvm-test-suite#684. Failures were not caused by this patch, but rather by misconfiguration of those tests |
Now it traverses reversed call graph by BFS algorithm from
__devicelib_assert_fail function up to SPIR kernels.