[libclc] Avoid out-of-range float-to-int. #19144
Conversation
For a kernel such as
kernel void foo(__global double3 *z) {
double3 x = {0.6631661088,0.6612268107,0.1513627528};
int3 y = {-1980459213,-660855407,615708204};
*z = pown(x, y);
}
we were not storing anything to z, because the implementation of pown
relied on an floating-point-to-integer conversion where the
floating-point value was outside of the integer's range. Although in
LLVM IR we permit that operation so long as we end up ignoring its
result -- that is the general rule for poison -- one thing we are not
permitted to do is have conditional branches that depend on it, and
through the call to __clc_ldexp, we did have that.
To fix this, rather than changing expv at the end to INFINITY/0, we can
change v at the start to values that we know will produce INFINITY/0
without performing such out-of-range conversions.
Tested with
clang --target=nvptx64 -S -O3 -o - test.cl \
-Xclang -mlink-builtin-bitcode \
-Xclang runtimes/runtimes-bins/libclc/nvptx64--.bc
A grep showed that this exact same code existed in three more places, so
I changed it there too, though I did not do a broader search for other
similar code that potentially has the same problem.
(cherry picked from commit 6e37101f93b285d6f0c5313d4367089daeb7097f)
|
I've submitted this ahead of the next pulldown not only because this affects SYCL-CTS (for Native CPU), but also because it would be easy for the next pulldown to miss that in DPC++ this code needs to be updated in one place which does not exist in upstream LLVM, |
|
@intel/llvm-gatekeepers This is ready to be merged, thanks. |
|
What's the urgency to cherry-pick? |
I left a comment addressing that specifically:
|
Sorry, missed it (looked at the description only). |
|
@hvdijk are you ok with my description edit (last sentence)? |
|
Sure, that's fine, thanks. |
For a kernel such as
we were not storing anything to z, because the implementation of pown relied on an floating-point-to-integer conversion where the floating-point value was outside of the integer's range. Although in LLVM IR we permit that operation so long as we end up ignoring its result -- that is the general rule for poison -- one thing we are not permitted to do is have conditional branches that depend on it, and through the call to __clc_ldexp, we did have that.
To fix this, rather than changing expv at the end to INFINITY/0, we can change v at the start to values that we know will produce INFINITY/0 without performing such out-of-range conversions.
Tested with
A grep showed that this exact same code existed in three more places, so I changed it there too, though I did not do a broader search for other similar code that potentially has the same problem.
(cherry picked from commit 46ee7f1, with a minor downstream-only follow-up in
libclc/libspirv/lib/generic/math/clc_rootn.cl)