[NFC][SYCL] Add few vec tests to document current behavior
#17720
Conversation
These will be updated as I upload PRs to implement recent specification changes.
aelovikov-intel
had a problem deploying
to
WindowsCILock
GitHub Actions
Error
| // RUN: %clangxx -fsycl -fsyntax-only %s -fpreview-breaking-changes | ||
| // RUN: %clangxx -fsycl -fsyntax-only %s | ||
|
|
||
| #include <sycl/sycl.hpp> |
There was a problem hiding this comment.
Can we include the header with only sycl::vec type declarations (+half)?
There was a problem hiding this comment.
We do have such policy for sycl/test-e2e but not here. IMO, it's wrong to selectively enforce it like that. If someone sees much value in not using sycl/sycl.hpp under sycl/test, then we need to clean up all existing tests and add a check that it's not added in new tests (like what I did for e2e tests).
There was a problem hiding this comment.
IMO, it's wrong to selectively enforce it like that.
What do you mean by "selectively" enforce? Isn't it a general practice to write the smallest test. Basically, your test aims to test vector class functionality but instead includes the whole SYCL library. What is the value in that? The negative effect of including sycl/sycl.hpp is overhead on parsing a lot of C++ code unrelated to the tested functionality. Considering that developers are running these tests multiple times and CI is running these tests a lot, removing unnecessary overhead seems like a win to me.
If someone sees much value in not using
sycl/sycl.hppundersycl/test, then we need to clean up all existing tests and add a check that it's not added in new tests (like what I did for e2e tests).
I agree that we should enforce good practices either through code review or with tooling.
There was a problem hiding this comment.
I don't think that individual non-official include is strictly better than sycl/sycl.hpp (because otherwise we wouldn't have had a similar discussion when I was proposing the opposite - using detail/core.hpp vs sycl/sycl.hpp in e2e tests). As such, without documented guidelines, it should be author's choice.
aelovikov-intel
temporarily deployed
to
WindowsCILock
GitHub Actions
Inactive
aelovikov-intel
temporarily deployed
to
WindowsCILock
GitHub Actions
Inactive
aelovikov-intel
temporarily deployed
to
WindowsCILock
GitHub Actions
Inactive
aelovikov-intel
temporarily deployed
to
WindowsCILock
GitHub Actions
Inactive
| // IN_PREVIEW_ONLY condition<> | ||
| // EXCEPT_IN_PREVIEW condition<> | ||
|
|
There was a problem hiding this comment.
Moving discussion from #17712 (comment) to here.
@uditagarwal97 , can you review please?
aelovikov-intel
temporarily deployed
to
WindowsCILock
GitHub Actions
Inactive
aelovikov-intel
temporarily deployed
to
WindowsCILock
GitHub Actions
Inactive
…7720) These will be updated as I upload PRs to implement recent specification changes.
These will be updated as I upload PRs to implement recent specification changes.