[sycl-post-link] Add device image property for assert feature #3881
Conversation
A property with the name of the kernel is added whenever the kernel uses assert. Details: https://github.com/intel/llvm/blob/sycl/sycl/doc/Assert.md#online-linking-fallback-__devicelib_assert_fail
vmaksimo
requested review from
AlexeySachkov,
kbobrovs and
mlychkov
as code owners
| // If we know, that this function does not contain assert, we still | ||
| // should investigate another instructions in the function. | ||
| if (!hasAssertionInCallGraphMap[CF]) | ||
| continue; |
There was a problem hiding this comment.
This kind of defeats the purpose of the cache, i.e. if we have a huge sub-graph which is known not to have an assertion and it is used from a several kernels, than we would traverse it fully several times.
If I understand correctly, this "if" is needed, because we go upwards by call stack to mark all functions as ones that definitely do not call assert. Technically, "definitely" word is not correct here: if we have a function which calls two other functions and we have only traversed a single call tree and didn't find an assertion, then we can't say that the top-level function definitely don't call assert - it only might not call assert.
A few ideas how to fix this:
- move upwards only until we are sure that all callees of the current function do not call an assertion. As soon as we have encountered a branch in a call three for which the answer is not known, we stop there. Unfortunately, this is likely to require additional data to store or additional actions to perform, so I'm not sure if it worth it.
- during the first traverse, only put
truevalues into the map, then perform a second graph traversal and mark everything you haven't marked in the first one withfalse
Since assert is an extension, I would expect more kernels/functions without it than kernels/functions with it. Therefore, I think that having an early exit when we know that particular sub-tree does not have an assert is important optimization.
However, this is not a functional problem and if @s-kanaev would like to get this patch sooner, than we can postpone optimizing our graph traversal and do that in a separate PR
There was a problem hiding this comment.
I'll work on further graph traversal optimization and apply it in a separate PR
| if (hasAssertionInCallGraphMap.count(CF)) { | ||
| // If we know, that this function does not contain assert, we still | ||
| // should investigate another instructions in the function. | ||
| if (!hasAssertionInCallGraphMap[CF]) |
There was a problem hiding this comment.
map::find method may be used to search CF in hasAssertionInCallGraphMap once here.
* upstream/sycl: (776 commits) Align CMake requirements with upstream. (intel#3928) [SYCL] Deprecate [[intel::reqd_work_group_size]] attribute spelling (intel#3927) [SYCL] Fix post commit after PR 2292 (intel#3939) {SYCL][PI][L0] - Eliminate std::string construction/destruction overhead. (intel#3931) [ESIMD] Overloading sycl sin,cos,exp,log functions for ESIMD arguments (intel#3717) [sycl-post-link] Add device image property for assert feature (intel#3881) [SYCL] Split read/write lockings (intel#2292) Handle OpSpecConstantOp with Select Handle OpSpecConstantOp with SMod Add tests for SConvert, UConvert, BitCast OpSpecConstantOp Fix attachment of decoration to spec constants Implement support for dynamic memmove Align clang-tidy/format versions to LLVM version Handle OpSpecConstantOp for integer comparisons Handle OpSpecConstantOp for SNegate, Not, and LogicalNot Extend OpSpecConstantOp testing for initializers Use IRBuilder for folding Fix translating of compile unit Support buffers in LinalgFoldUnitExtentDims [gn build] Port d0a5d86 ...
A property with the name of the kernel is added whenever the kernel uses assert.
Details: https://github.com/intel/llvm/blob/sycl/sycl/doc/Assert.md#online-linking-fallback-__devicelib_assert_fail