[SYCL][Reduction] Avoid implicit atomic64 requirements #9070
Conversation
Some of the existing reduction strategies use atomic operations on partial reduction results. However, for reductions on 64-bit values this implicitly adds a requirement that the corresponding device supports aspect::atomic64. This commit adds additional logic to select different strategies based on this. Note; one of these naively chooses another strategy if the type is 64-bit without checking the support at runtime. Follow-up patches should refactor the strategy selection, allowing this to do appropriate runtime checks. Signed-off-by: Larsen, Steffen <[email protected]>
Signed-off-by: Larsen, Steffen <[email protected]>
| // TODO: When aspect requirements are added to testing, this test could be set | ||
| // to require that atomic64 is NOT supported, to limit how frequently the | ||
| // test is run. However, it should work on devices that support atomic64 | ||
| // as well. |
There was a problem hiding this comment.
I'd suggest adding runtime aspect check for that here in this PR with the expectation that we'll uplift it to // REQUIRES once the support is added to the llvm.lit.cfg.py.
I also think that a slightly better option would be to "unit-test" the dynamic strategy selector once it's implemented (i.e., don't even run the actual reduction kernel).
There was a problem hiding this comment.
I'd suggest adding runtime aspect check for that here in this PR with the expectation that we'll uplift it to
// REQUIRESonce the support is added to thellvm.lit.cfg.py.
We could do a runtime check if you prefer. The test is still valid, so this TODO is more of a suggestion than a requirement really.
I also think that a slightly better option would be to "unit-test" the dynamic strategy selector once it's implemented (i.e., don't even run the actual reduction kernel).
I agree that would be better, but sadly the unittests don't compile the kernels, so we would have no way of knowing if the corresponding kernels had any requirements. We could set the requirements ourselves, but that really defeats the purpose of the tests.
Signed-off-by: Larsen, Steffen <[email protected]>
Some of the existing reduction strategies use atomic operations on partial reduction results. However, for reductions on 64-bit values this implicitly adds a requirement that the corresponding device supports aspect::atomic64. This commit adds additional logic to select different strategies based on this.
Note; one of these naively chooses another strategy if the type is 64-bit without checking the support at runtime. Follow-up patches should refactor the strategy selection, allowing this to do appropriate runtime checks.