[SYCL] Enhance specialization constants testing.#12647
[SYCL] Enhance specialization constants testing.#12647aelovikov-intel merged 6 commits intointel:syclfrom
Conversation
Signed-off-by: Marcos Maronas <marcos.maronas@intel.com>
Signed-off-by: Marcos Maronas <marcos.maronas@intel.com>
Signed-off-by: Marcos Maronas <marcos.maronas@intel.com>
maarquitos14
temporarily deployed
to
WindowsCILock
GitHub Actions
Inactive
maarquitos14
temporarily deployed
to
WindowsCILock
GitHub Actions
Inactive
…nstants-enhanced-testing
maarquitos14
temporarily deployed
to
WindowsCILock
GitHub Actions
Inactive
maarquitos14
temporarily deployed
to
WindowsCILock
GitHub Actions
Inactive
|
Friendly ping @AlexeySachkov |
AlexeySachkov
left a comment
AlexeySachkov
left a comment
There was a problem hiding this comment.
Overall, I don't have objections to the approach and I'm fine with merging the PR. However, I still want to record some of my thoughts on the matter:
Testing through debug macro will only help a single pass, but won't provide a generic way of testing properties we emit in a human-readable way. We may also consider adding some mechanisms to tweak how properties are emitted so we can print them in human-readable form for tests. With such infrastructure we would need less changes overall to test that various properties are properly emitted.
At the same time, debug prints within a pass will likely provide better representation for domain-specific info, which we could use to write better tests. This pass is a good example: we print a byte array as a series of triplets, which greatly improves readability of the test.
llvm/test/tools/sycl-post-link/spec-constants/SYCL-2020-zeroinitializer-array-of-arrays.ll
Outdated
Show resolved
Hide resolved
| std::fill_n(std::back_inserter(DefaultValues), NumBytes, 0); | ||
| Offset += NumBytes; | ||
| // Print tuple {Offset, Size, DefaultValue}. | ||
| LLVM_DEBUG(dbgs() << "{" << Offset - NumBytes << ", " << NumBytes << ", " |
There was a problem hiding this comment.
I don't have any data about how often folks use LLVM_DEBUG to quickly hack some debugging messages (I personally just use plain llvm::outs() for that), but if we know of someone who does that, then we should probably use DEBUG_WITH_TYPE to allow both generic debug messages and messages which are specific to certain functionality .
With the current approach, if anyone want's to see some printouts from the pass using LLVM_DEBUG mechanism, they may have to filter out this output because it is irrelevant to their investigation
There was a problem hiding this comment.
I think they could easily do that by defining their own DEBUG_TYPE --it just takes 1 line-- and then pass --debug-only=<custom_type> . Also, I grepped LLVM_DEBUG and there are +11k occurrences, so it's definitely better to filter only their own option if they want to quickly hack some own debugging messages.
There was a problem hiding this comment.
Yeah, DEBUG_WITH_TYPE is handy when you want to have several different filters in a single TU. Otherwise, DEBUG_TYPE is the way to go.
Signed-off-by: Marcos Maronas <marcos.maronas@intel.com>
maarquitos14
temporarily deployed
to
WindowsCILock
GitHub Actions
Inactive
maarquitos14
had a problem deploying
to
WindowsCILock
GitHub Actions
Failure
llvm/test/tools/sycl-post-link/spec-constants/composite-default-value-padding.ll
Outdated
Show resolved
Hide resolved
Signed-off-by: Marcos Maronas <marcos.maronas@intel.com>
maarquitos14
temporarily deployed
to
WindowsCILock
GitHub Actions
Inactive
maarquitos14
had a problem deploying
to
WindowsCILock
GitHub Actions
Failure
|
The CI failure was already found in #12791, and it's unrelated to this PR. This is ready to merge @intel/llvm-gatekeepers |
|
This breaks post-commit: https://github.com/intel/llvm/actions/runs/8022089783. Please fix asap or revert. |
Recent changes made to specialization constants tests relied on `--debug-only` option, which is only enabled when assertions are enabled. This commit guards the commands using such an option to be run only when assertions are enabled. This should fix the post-commit CI failure observed after #12647 was merged. --------- Signed-off-by: Marcos Maronas <marcos.maronas@intel.com>
5 participants
Introduce logging ability to
SpecConstantsPassthroughLLVM_DEBUGmacro so that we can enhance testing.