[SYCL][E2E] Print features detected by lit#16866
Conversation
c9c53d8 to
3151a52
Compare
|
This is a bit controversial as the output is noisy. Feel free to vote via thumb up/down. |
againull
left a comment
There was a problem hiding this comment.
from the CI log doesn't look noisy
sycl/test-e2e/lit.cfg.py
Outdated
| ("%clang", " " + config.dpcpp_compiler + " " + config.c_flags) | ||
| ) | ||
|
|
||
| lit_config.note( |
There was a problem hiding this comment.
How about we print all features only if --verbose / -v is specified?
There was a problem hiding this comment.
I don't think it would be useful that much. This PR only makes sense if we enable it by default in all our CI jobs, meaning the noise will be there.
There was a problem hiding this comment.
I don't think it would be useful that much. This PR only makes sense if we enable it by default in all our CI jobs, meaning the noise will be there.
IIUC, we already pass -v to llvm-lit in our CI jobs: https://github.com/intel/llvm/blob/sycl/devops/actions/run-tests/e2e/action.yml#L62.
I don't mind printing features in CI jobs as they would be useful for debugging (it's not that noisy :)). My preference would be to not print features unnecessarily when we invoke llvm-lit locally. We already print device aspects unconditionally and llvm-lit's output (even without --verbose) can be big when we have multiple devices in the environment.
|
I think this is a good change. I do understand the concern about noisy output, but the information (in particular the common aspects) is generally hard to otherwise gather and has an important impact on how the testing is done, so the noise at the beginning of the already fairly verbose LIT output seems worth it to me. |
|
Seems fine to me too, the location of the extra output is localized. |
4b6317d to
9a1bccf
Compare
No description provided.