Skip to content

Commit 054bc27

Browse files
[SYCL] Fix min/max known_identity<> wrong when -ffast-math is used. (#17028)
Technically a workaround, this fixes the problem with `known_identity<minOp/maxOp>` being incorrect when using `-ffast-math`. It works because we have a `__FAST_MATH__` macro that is defined when that flag is used. Better, would be to detect `-fno-honor-infinities`. Opening a JIRA/discussion for that, but this is a big improvement in the interim. See discussion #13813
1 parent d6ae29c commit 054bc27

File tree

2 files changed

+84
-0
lines changed

2 files changed

+84
-0
lines changed

sycl/include/sycl/known_identity.hpp

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -258,10 +258,24 @@ 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)
271+
static constexpr AccumulatorT value =
272+
(std::numeric_limits<AccumulatorT>::max)();
273+
#else
261274
static constexpr AccumulatorT value = static_cast<AccumulatorT>(
262275
std::numeric_limits<AccumulatorT>::has_infinity
263276
? std::numeric_limits<AccumulatorT>::infinity()
264277
: (std::numeric_limits<AccumulatorT>::max)());
278+
#endif
265279
};
266280

267281
#if (!defined(_HAS_STD_BYTE) || _HAS_STD_BYTE != 0)
@@ -295,11 +309,19 @@ template <typename BinaryOperation, typename AccumulatorT>
295309
struct known_identity_impl<BinaryOperation, AccumulatorT,
296310
std::enable_if_t<IsMaximumIdentityOp<
297311
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__
316+
static constexpr AccumulatorT value =
317+
(std::numeric_limits<AccumulatorT>::lowest)();
318+
#else
298319
static constexpr AccumulatorT value = static_cast<AccumulatorT>(
299320
std::numeric_limits<AccumulatorT>::has_infinity
300321
? static_cast<AccumulatorT>(
301322
-std::numeric_limits<AccumulatorT>::infinity())
302323
: std::numeric_limits<AccumulatorT>::lowest());
324+
#endif
303325
};
304326

305327
#if (!defined(_HAS_STD_BYTE) || _HAS_STD_BYTE != 0)
Lines changed: 62 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,62 @@
1+
/*
2+
See https://github.com/intel/llvm/issues/13813
3+
4+
-ffast-math implies -fno-honor-infinities. In this case, the known_identity
5+
for sycl::minimum<T> cannot be inifinity (which will be 0), but must instead
6+
be the max<T> . Otherwise, reducing sycl::minimum<T>
7+
over {4.0f, 1.0f, 3.0f, 2.0f} will return 0.0 instead of 1.0.
8+
*/
9+
10+
// RUN: %clangxx -fsycl -fsyntax-only %s
11+
// RUN: %clangxx -fsycl -fsyntax-only %s -ffast-math
12+
// RUN: %clangxx -fsycl -fsyntax-only %s -fno-fast-math
13+
14+
#include <sycl/ext/oneapi/bfloat16.hpp>
15+
#include <sycl/sycl.hpp>
16+
17+
#include <cassert>
18+
#include <limits>
19+
20+
template <typename OperandT> void test_known_identity_min() {
21+
constexpr OperandT identity =
22+
sycl::known_identity_v<sycl::minimum<OperandT>, OperandT>;
23+
#ifdef __FAST_MATH__
24+
constexpr OperandT expected = std::numeric_limits<OperandT>::max();
25+
#else
26+
constexpr OperandT expected = std::numeric_limits<OperandT>::infinity();
27+
#endif
28+
29+
static_assert(identity == expected,
30+
"Known identity for sycl::minimum<T> is incorrect");
31+
}
32+
33+
template <typename OperandT> void test_known_identity_max() {
34+
constexpr OperandT identity =
35+
sycl::known_identity_v<sycl::maximum<OperandT>, OperandT>;
36+
#ifdef __FAST_MATH__
37+
constexpr OperandT expected = std::numeric_limits<OperandT>::lowest();
38+
#else
39+
constexpr OperandT expected =
40+
-std::numeric_limits<OperandT>::infinity(); // negative infinity
41+
#endif
42+
43+
static_assert(identity == expected,
44+
"Known identity for sycl::maximum<T> is incorrect");
45+
}
46+
47+
int main() {
48+
test_known_identity_min<float>();
49+
test_known_identity_min<double>();
50+
test_known_identity_min<sycl::half>();
51+
52+
test_known_identity_max<float>();
53+
test_known_identity_max<double>();
54+
test_known_identity_max<sycl::half>();
55+
56+
// bfloat16 seems to be missing constexpr == which is used above.
57+
// commenting out until fixed.
58+
// test_known_identity_min<sycl::ext::oneapi::bfloat16>();
59+
// test_known_identity_max<sycl::ext::oneapi::bfloat16>();
60+
61+
return 0;
62+
}

0 commit comments

Comments
 (0)