Skip to content

Commit 8614a89

Browse files
[SYCL] tighten known_identity to finite_math macro (#18247)
Some time ago we fixed a bug in `sycl::known_identity` where it was incorrectly using `std::numeric_limits<T>::infinity()` even in cases where `::infinity()` should not have been available. At the time, the fix was to simply test for `__FAST_MATH__`. That worked, but was not ideal. Now we are tightening this to use the `__FINITE_MATH_ONLY__` macro which is more accurate. see #13813 `__FINITE_MATH_ONLY__` will be set when both the `-fno-honor-infinities` and `-fno-honor-nans` flags are used. It was felt by the FE team that there is no reasonable case where one of the flags would be used without the other and therefore `__FINITE_MATH_ONLY__` is sufficient for this problem ( and superior to the `__FAST_MATH__` macro we were using before). --------- Signed-off-by: Chris Perkins <chris.perkins@intel.com>
1 parent 1e81dea commit 8614a89

File tree

2 files changed

+18
-20
lines changed

2 files changed

+18
-20
lines changed

sycl/include/sycl/known_identity.hpp

Lines changed: 12 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -258,16 +258,16 @@ template <typename BinaryOperation, typename AccumulatorT>
258258
struct known_identity_impl<BinaryOperation, AccumulatorT,
259259
std::enable_if_t<IsMinimumIdentityOp<
260260
AccumulatorT, BinaryOperation>::value>> {
261-
// TODO: detect -fno-honor-infinities instead of -ffast-math
262-
// See https://github.com/intel/llvm/issues/13813
263-
// This workaround is a vast improvement, but still falls short.
264-
// To correct it properly, we need to detect -fno-honor-infinities usage,
265-
// perhaps via a driver inserted macro.
266-
// See similar below in known_identity_impl<IsMaximumIdentityOp>
267-
#ifdef __FAST_MATH__
268-
// -ffast-math implies -fno-honor-infinities,
269-
// but neither affect ::has_infinity (which is correct behavior, if
270-
// unexpected)
261+
262+
#if defined(__FINITE_MATH_ONLY__) && (__FINITE_MATH_ONLY__ == 1)
263+
// Finite math only (-ffast-math, -fno-honor-infinities) improves
264+
// performance, but does not affect ::has_infinity (which is correct behavior,
265+
// if unexpected). Use ::max() instead of ::infinity().
266+
// Note that if someone uses -fno-honor-infinities WITHOUT -fno-honor-nans
267+
// they'll end up using ::infinity(). There is no reasonable case where one of
268+
// the flags would be used without the other and therefore
269+
// __FINITE_MATH_ONLY__ is sufficient for this problem ( and superior to the
270+
// __FAST_MATH__ macro we were using before).
271271
static constexpr AccumulatorT value =
272272
(std::numeric_limits<AccumulatorT>::max)();
273273
#else
@@ -309,10 +309,8 @@ template <typename BinaryOperation, typename AccumulatorT>
309309
struct known_identity_impl<BinaryOperation, AccumulatorT,
310310
std::enable_if_t<IsMaximumIdentityOp<
311311
AccumulatorT, BinaryOperation>::value>> {
312-
// TODO: detect -fno-honor-infinities instead of -ffast-math
313-
// See https://github.com/intel/llvm/issues/13813
314-
// and comments above in known_identity_impl<IsMinimumIdentityOp>
315-
#ifdef __FAST_MATH__
312+
// See comment above in known_identity_impl<IsMinimumIdentityOp>
313+
#if defined(__FINITE_MATH_ONLY__) && (__FINITE_MATH_ONLY__ == 1)
316314
static constexpr AccumulatorT value =
317315
(std::numeric_limits<AccumulatorT>::lowest)();
318316
#else

sycl/test/basic_tests/reduction/infinite_identity.cpp

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,4 @@
11
/*
2-
See https://github.com/intel/llvm/issues/13813
3-
42
-ffast-math implies -fno-honor-infinities. In this case, the known_identity
53
for sycl::minimum<T> cannot be inifinity (which will be 0), but must instead
64
be the max<T> . Otherwise, reducing sycl::minimum<T>
@@ -10,6 +8,8 @@
108
// RUN: %clangxx -fsycl -fsyntax-only %s
119
// RUN: %clangxx -fsycl -fsyntax-only %s -ffast-math
1210
// RUN: %clangxx -fsycl -fsyntax-only %s -fno-fast-math
11+
// RUN: %clangxx -fsycl -fsyntax-only %s -fno-fast-math -fno-honor-infinities -fno-honor-nans
12+
// RUN: %clangxx -fsycl -fsyntax-only %s -ffast-math -fhonor-infinities -fhonor-nans
1313

1414
#include <sycl/ext/oneapi/bfloat16.hpp>
1515
#include <sycl/sycl.hpp>
@@ -20,7 +20,7 @@
2020
template <typename OperandT> void test_known_identity_min() {
2121
constexpr OperandT identity =
2222
sycl::known_identity_v<sycl::minimum<OperandT>, OperandT>;
23-
#ifdef __FAST_MATH__
23+
#if defined(__FINITE_MATH_ONLY__) && (__FINITE_MATH_ONLY__ == 1)
2424
constexpr OperandT expected = std::numeric_limits<OperandT>::max();
2525
#else
2626
constexpr OperandT expected = std::numeric_limits<OperandT>::infinity();
@@ -33,11 +33,11 @@ template <typename OperandT> void test_known_identity_min() {
3333
template <typename OperandT> void test_known_identity_max() {
3434
constexpr OperandT identity =
3535
sycl::known_identity_v<sycl::maximum<OperandT>, OperandT>;
36-
#ifdef __FAST_MATH__
36+
#if defined(__FINITE_MATH_ONLY__) && (__FINITE_MATH_ONLY__ == 1)
3737
constexpr OperandT expected = std::numeric_limits<OperandT>::lowest();
3838
#else
39-
constexpr OperandT expected =
40-
-std::numeric_limits<OperandT>::infinity(); // negative infinity
39+
// negative infinity
40+
constexpr OperandT expected = -std::numeric_limits<OperandT>::infinity();
4141
#endif
4242

4343
static_assert(identity == expected,

0 commit comments

Comments
 (0)