[PATCH] RISC-V: Fix reduc_strict_run-1 test case.

[PATCH] RISC-V: Fix reduc_strict_run-1 test case.

[Robin Dapp]

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 <math.h>
+
+#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.

[Jeff Law]

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: Re: [PATCH] RISC-V: Fix reduc_strict_run-1 test case.

[juzhe.zhong]

[-- Attachment #1: Type: text/plain, Size: 1029 bytes --]

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.zhong@rivai.ai
 
From: Jeff Law
Date: 2023-08-16 09:13
To: Robin Dapp; gcc-patches; palmer; Kito Cheng; juzhe.zhong@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.

[Jeff Law]

On 8/15/23 19:21, juzhe.zhong@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: [PATCH] RISC-V: Fix reduc_strict_run-1 test case.

[juzhe.zhong]

[-- Attachment #1: Type: text/plain, Size: 1816 bytes --]

LGTM



juzhe.zhong@rivai.ai
 
From: Robin Dapp
Date: 2023-08-15 23:49
To: gcc-patches; palmer; Kito Cheng; jeffreyalaw; juzhe.zhong@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 <math.h>
+
+#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.

[Robin Dapp]

> 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.

[Jeff Law]

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.

[Palmer Dabbelt]

On Wed, 16 Aug 2023 15:59:13 PDT (-0700), jeffreyalaw@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.

[Robin Dapp]

> 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