Re: [PATCH] RISC-V: Fix reduc_strict_run-1 test case.
> I'm not opposed to merging the test change, but I couldn't figure out > where in C the implicit conversion was coming from: as far as I can > tell the macros don't introduce any (it's "return _float16 * > _float16"), I'd had the patch open since last night but couldn't > figure it out. > > We get a bunch of half->single->half converting in the generated > assembly that smelled like we had a bug somewhere else, sorry if I'm > just missing something... Yes, good point, my explanation was wrong again. What really (TM) happens is that the equality comparison, in presence of _Float16 emulation(!), performs an extension to float/double for its arguments. So if (res != r * q) is if ((float)res (float)!= (float)(r * q)) Now, (r * q) is also implicitly computed in float. Because the comparison requires a float argument, there is no intermediate conversion back to _Float16 and the value is more accurate than it would be in _Float16. res, however, despite being calculated in float as well, is converted to _Float16 for the function return or rather the assignment to "res". Therefore it is less accurate than (r * q) and the comparison fails. So, what would also help, even though it's not obvious at first sight is: TYPE res = reduc_plus_##TYPE (a, b); \ -if (res != r * q) \ +TYPE ref = r * q; \ +if (res != ref)\ __builtin_abort (); \ } This does not happen with proper _zfh because the operations are done in _Float16 precision then. BTW such kinds of non-obvious problems are the reason why I split off _zvfh run tests into separate files right away. Regards Robin
Re: [PATCH] RISC-V: Fix reduc_strict_run-1 test case.
On Wed, 16 Aug 2023 15:59:13 PDT (-0700), jeffreya...@gmail.com wrote: On 8/16/23 07:50, Robin Dapp wrote: But if it's a float16 precision issue then I would have expected both the computations for the lhs and rhs values to have suffered similarly. Yeah, right. I didn't look closely enough. The problem is not the reduction but the additional return-value conversion that is omitted when calculating the reference value inline. The attached is simpler and does the trick. Regards Robin Subject: [PATCH v2] RISC-V: Fix reduc_strict_run-1 test case. This patch fixes the reduc_strict_run-1 testcase by converting the reference value to double and back to the tested type. Without that omitted the implicit return-value conversion and would produce a different result for _Float16. gcc/testsuite/ChangeLog: * gcc.target/riscv/rvv/autovec/reduc/reduc_strict_run-1.c: Perform type -> double -> type conversion for reference value. OK I'm not opposed to merging the test change, but I couldn't figure out where in C the implicit conversion was coming from: as far as I can tell the macros don't introduce any (it's "return _float16 * _float16"), I'd had the patch open since last night but couldn't figure it out. We get a bunch of half->single->half converting in the generated assembly that smelled like we had a bug somewhere else, sorry if I'm just missing something... jeff
Re: [PATCH] RISC-V: Fix reduc_strict_run-1 test case.
On 8/16/23 07:50, Robin Dapp wrote: But if it's a float16 precision issue then I would have expected both the computations for the lhs and rhs values to have suffered similarly. Yeah, right. I didn't look closely enough. The problem is not the reduction but the additional return-value conversion that is omitted when calculating the reference value inline. The attached is simpler and does the trick. Regards Robin Subject: [PATCH v2] RISC-V: Fix reduc_strict_run-1 test case. This patch fixes the reduc_strict_run-1 testcase by converting the reference value to double and back to the tested type. Without that omitted the implicit return-value conversion and would produce a different result for _Float16. gcc/testsuite/ChangeLog: * gcc.target/riscv/rvv/autovec/reduc/reduc_strict_run-1.c: Perform type -> double -> type conversion for reference value. OK jeff
Re: [PATCH] RISC-V: Fix reduc_strict_run-1 test case.
> But if it's a float16 precision issue then I would have expected both
> the computations for the lhs and rhs values to have suffered
> similarly.
Yeah, right. I didn't look closely enough. The problem is not the
reduction but the additional return-value conversion that is omitted
when calculating the reference value inline.
The attached is simpler and does the trick.
Regards
Robin
Subject: [PATCH v2] RISC-V: Fix reduc_strict_run-1 test case.
This patch fixes the reduc_strict_run-1 testcase by converting
the reference value to double and back to the tested type.
Without that omitted the implicit return-value conversion and
would produce a different result for _Float16.
gcc/testsuite/ChangeLog:
* gcc.target/riscv/rvv/autovec/reduc/reduc_strict_run-1.c:
Perform type -> double -> type conversion for reference value.
---
.../gcc.target/riscv/rvv/autovec/reduc/reduc_strict_run-1.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git
a/gcc/testsuite/gcc.target/riscv/rvv/autovec/reduc/reduc_strict_run-1.c
b/gcc/testsuite/gcc.target/riscv/rvv/autovec/reduc/reduc_strict_run-1.c
index 516be97e9eb..d5a544b1cc9 100644
--- a/gcc/testsuite/gcc.target/riscv/rvv/autovec/reduc/reduc_strict_run-1.c
+++ b/gcc/testsuite/gcc.target/riscv/rvv/autovec/reduc/reduc_strict_run-1.c
@@ -17,7 +17,7 @@
asm volatile ("" ::: "memory"); \
}\
TYPE res = reduc_plus_##TYPE (a, b); \
-if (res != r * q) \
+if (res != (TYPE)(double)(r * q)) \
__builtin_abort (); \
}
--
2.41.0
Re: [PATCH] RISC-V: Fix reduc_strict_run-1 test case.
LGTM
juzhe.zh...@rivai.ai
From: Robin Dapp
Date: 2023-08-15 23:49
To: gcc-patches; palmer; Kito Cheng; jeffreyalaw; juzhe.zh...@rivai.ai
CC: rdapp.gcc
Subject: [PATCH] RISC-V: Fix reduc_strict_run-1 test case.
Hi,
this patch changes the equality check for the reduc_strict_run-1
testcase from == to fabs () < EPS. The FAIL only occurs with
_Float16 but I'd argue approximate equality is preferable for all
float modes.
Regards
Robin
gcc/testsuite/ChangeLog:
* gcc.target/riscv/rvv/autovec/reduc/reduc_strict_run-1.c:
Check float equality with fabs < EPS.
---
.../riscv/rvv/autovec/reduc/reduc_strict_run-1.c | 9 ++---
1 file changed, 6 insertions(+), 3 deletions(-)
diff --git
a/gcc/testsuite/gcc.target/riscv/rvv/autovec/reduc/reduc_strict_run-1.c
b/gcc/testsuite/gcc.target/riscv/rvv/autovec/reduc/reduc_strict_run-1.c
index 516be97e9eb..93efe2c4333 100644
--- a/gcc/testsuite/gcc.target/riscv/rvv/autovec/reduc/reduc_strict_run-1.c
+++ b/gcc/testsuite/gcc.target/riscv/rvv/autovec/reduc/reduc_strict_run-1.c
@@ -2,6 +2,9 @@
/* { dg-additional-options "--param=riscv-autovec-preference=scalable
-fno-vect-cost-model" } */
#include "reduc_strict-1.c"
+#include
+
+#define EPS 1e-2
#define TEST_REDUC_PLUS(TYPE) \
{ \
@@ -10,14 +13,14 @@
TYPE r = 0, q = 3; \
for (int i = 0; i < NUM_ELEMS (TYPE); i++) \
{ \
- a[i] = (i * 0.1) * (i & 1 ? 1 : -1); \
- b[i] = (i * 0.3) * (i & 1 ? 1 : -1); \
+ a[i] = (i * 0.01) * (i & 1 ? 1 : -1); \
+ b[i] = (i * 0.03) * (i & 1 ? 1 : -1); \
r += a[i]; \
q -= b[i]; \
asm volatile ("" ::: "memory"); \
} \
TYPE res = reduc_plus_##TYPE (a, b); \
-if (res != r * q) \
+if (fabs (res - r * q) > EPS) \
__builtin_abort (); \
}
--
2.41.0
Re: [PATCH] RISC-V: Fix reduc_strict_run-1 test case.
On 8/15/23 19:21, juzhe.zh...@rivai.ai wrote: For float/double, the in-order fold-left reduction produced the same result as scalar codes. But for _Float16 is not, I think the issue is not the reduction issue, is float 16 precision issue. But if it's a float16 precision issue then I would have expected both the computations for the lhs and rhs values to have suffered similarly. But if you're confident it's OK, then I won't object. jeff
Re: Re: [PATCH] RISC-V: Fix reduc_strict_run-1 test case.
For float/double, the in-order fold-left reduction produced the same result as scalar codes. But for _Float16 is not, I think the issue is not the reduction issue, is float 16 precision issue. Thanks. juzhe.zh...@rivai.ai From: Jeff Law Date: 2023-08-16 09:13 To: Robin Dapp; gcc-patches; palmer; Kito Cheng; juzhe.zh...@rivai.ai Subject: Re: [PATCH] RISC-V: Fix reduc_strict_run-1 test case. On 8/15/23 09:49, Robin Dapp wrote: > Hi, > > this patch changes the equality check for the reduc_strict_run-1 > testcase from == to fabs () < EPS. The FAIL only occurs with > _Float16 but I'd argue approximate equality is preferable for all > float modes. > > Regards > Robin > > gcc/testsuite/ChangeLog: > > * gcc.target/riscv/rvv/autovec/reduc/reduc_strict_run-1.c: > Check float equality with fabs < EPS. Generally agree with using an EPS test. The question is shouldn't a fold-left reduction be done in-order and produce the same result as a scalar equivalent? Jeff
Re: [PATCH] RISC-V: Fix reduc_strict_run-1 test case.
On 8/15/23 09:49, Robin Dapp wrote: Hi, this patch changes the equality check for the reduc_strict_run-1 testcase from == to fabs () < EPS. The FAIL only occurs with _Float16 but I'd argue approximate equality is preferable for all float modes. Regards Robin gcc/testsuite/ChangeLog: * gcc.target/riscv/rvv/autovec/reduc/reduc_strict_run-1.c: Check float equality with fabs < EPS. Generally agree with using an EPS test. The question is shouldn't a fold-left reduction be done in-order and produce the same result as a scalar equivalent? Jeff
[PATCH] RISC-V: Fix reduc_strict_run-1 test case.
Hi,
this patch changes the equality check for the reduc_strict_run-1
testcase from == to fabs () < EPS. The FAIL only occurs with
_Float16 but I'd argue approximate equality is preferable for all
float modes.
Regards
Robin
gcc/testsuite/ChangeLog:
* gcc.target/riscv/rvv/autovec/reduc/reduc_strict_run-1.c:
Check float equality with fabs < EPS.
---
.../riscv/rvv/autovec/reduc/reduc_strict_run-1.c | 9 ++---
1 file changed, 6 insertions(+), 3 deletions(-)
diff --git
a/gcc/testsuite/gcc.target/riscv/rvv/autovec/reduc/reduc_strict_run-1.c
b/gcc/testsuite/gcc.target/riscv/rvv/autovec/reduc/reduc_strict_run-1.c
index 516be97e9eb..93efe2c4333 100644
--- a/gcc/testsuite/gcc.target/riscv/rvv/autovec/reduc/reduc_strict_run-1.c
+++ b/gcc/testsuite/gcc.target/riscv/rvv/autovec/reduc/reduc_strict_run-1.c
@@ -2,6 +2,9 @@
/* { dg-additional-options "--param=riscv-autovec-preference=scalable
-fno-vect-cost-model" } */
#include "reduc_strict-1.c"
+#include
+
+#define EPS 1e-2
#define TEST_REDUC_PLUS(TYPE) \
{\
@@ -10,14 +13,14 @@
TYPE r = 0, q = 3; \
for (int i = 0; i < NUM_ELEMS (TYPE); i++) \
{\
- a[i] = (i * 0.1) * (i & 1 ? 1 : -1);\
- b[i] = (i * 0.3) * (i & 1 ? 1 : -1);\
+ a[i] = (i * 0.01) * (i & 1 ? 1 : -1); \
+ b[i] = (i * 0.03) * (i & 1 ? 1 : -1); \
r += a[i]; \
q -= b[i]; \
asm volatile ("" ::: "memory"); \
}\
TYPE res = reduc_plus_##TYPE (a, b); \
-if (res != r * q) \
+if (fabs (res - r * q) > EPS) \
__builtin_abort (); \
}
--
2.41.0
