[PATCH RESEND v3 0/2] add APIs to handle alternative sNaN propagation for fmax/fmin

[PATCH RESEND v3 0/2] add APIs to handle alternative sNaN propagation for fmax/fmin

[frank.chang]

From: Frank Chang <frank.chang@sifive.com>

In IEEE 754-2019, minNum, maxNum, minNumMag and maxNumMag are removed
and replaced with minimum, minimumNumber, maximum and maximumNumber.

minimumNumber/maximumNumber behavior for SNaN is changed to:
  * If both operands are NaNs, a QNaN is returned.
  * If either operand is a SNaN, an invalid operation exception is signaled,
    but unless both operands are NaNs, the SNaN is otherwise ignored and
    not converted to a QNaN.

This patchset add support of the above alternative sNaN propagation for
fmax/fmin, which is required by RISC-V floating-point v2.2.

Chih-Min Chao (2):
  softfloat: add APIs to handle alternative sNaN propagation for
    fmax/fmin
  target/riscv: change the api for single/double fmin/fmax

 fpu/softfloat-parts.c.inc | 19 +++++++++++++++++++
 fpu/softfloat.c           | 18 +++++++++++++-----
 include/fpu/softfloat.h   | 10 ++++++++++
 target/riscv/fpu_helper.c |  8 ++++----
 4 files changed, 46 insertions(+), 9 deletions(-)

--
2.25.1

[PATCH v3 1/2] softfloat: add APIs to handle alternative sNaN propagation for fmax/fmin

[frank.chang]

From: Chih-Min Chao <chihmin.chao@sifive.com>

For "fmax/fmin ft0, ft1, ft2" and if one of the inputs is sNaN,

  The original logic:
    Return NaN and set invalid flag if ft1 == sNaN || ft2 == sNan.

  The alternative path:
    Set invalid flag if ft1 == sNaN || ft2 == sNaN.
    Return NaN only if ft1 == NaN && ft2 == NaN.

The IEEE 754 spec allows both implementation and some architecture such
as riscv choose different defintions in two spec versions.
(riscv-spec-v2.2 use original version, riscv-spec-20191213 changes to
 alternative)

Signed-off-by: Chih-Min Chao <chihmin.chao@sifive.com>
Signed-off-by: Frank Chang <frank.chang@sifive.com>
---
 fpu/softfloat-parts.c.inc | 19 +++++++++++++++++++
 fpu/softfloat.c           | 18 +++++++++++++-----
 include/fpu/softfloat.h   | 10 ++++++++++
 3 files changed, 42 insertions(+), 5 deletions(-)

diff --git a/fpu/softfloat-parts.c.inc b/fpu/softfloat-parts.c.inc
index dddee92d6ee..a8d74624f5a 100644
--- a/fpu/softfloat-parts.c.inc
+++ b/fpu/softfloat-parts.c.inc
@@ -1227,6 +1227,25 @@ static FloatPartsN *partsN(minmax)(FloatPartsN *a, FloatPartsN *b,
             && (ab_mask & ~float_cmask_qnan)) {
             return is_nan(a->cls) ? b : a;
         }
+
+        /*
+         * In IEEE 754-2019, minNum, maxNum, minNumMag and maxNumMag
+         * are removed and replaced with minimum, minimumNumber, maximum
+         * and maximumNumber.
+         * minimumNumber/maximumNumber behavior for SNaN is changed to:
+         *   If both operands are NaNs, a QNaN is returned.
+         *   If either operand is a SNaN,
+         *   an invalid operation exception is signaled,
+         *   but unless both operands are NaNs,
+         *   the SNaN is otherwise ignored and not converted to a QNaN.
+         */
+        if (!(~flags & (minmax_isnum | minmax_snan_noprop))
+            && (ab_mask & float_cmask_snan)
+            && (ab_mask & ~float_cmask_anynan)) {
+            float_raise(float_flag_invalid, s);
+            return is_nan(a->cls) ? b : a;
+        }
+
         return parts_pick_nan(a, b, s);
     }
 
diff --git a/fpu/softfloat.c b/fpu/softfloat.c
index 6e769f990c2..eee65e9934c 100644
--- a/fpu/softfloat.c
+++ b/fpu/softfloat.c
@@ -436,6 +436,11 @@ enum {
     minmax_isnum = 2,
     /* Set for the IEEE 754-2008 minNumMag() and minNumMag() operations. */
     minmax_ismag = 4,
+    /*
+     * Set for the IEEE 754-2019 minimumNumber() maximumNumber() operations,
+     * without sNaN propagation.
+     */
+    minmax_snan_noprop = 8,
 };
 
 /* Simple helpers for checking if, or what kind of, NaN we have */
@@ -3927,11 +3932,14 @@ static float128 float128_minmax(float128 a, float128 b,
     { return type##_minmax(a, b, s, flags); }
 
 #define MINMAX_2(type) \
-    MINMAX_1(type, max, 0)                                      \
-    MINMAX_1(type, maxnum, minmax_isnum)                        \
-    MINMAX_1(type, maxnummag, minmax_isnum | minmax_ismag)      \
-    MINMAX_1(type, min, minmax_ismin)                           \
-    MINMAX_1(type, minnum, minmax_ismin | minmax_isnum)         \
+    MINMAX_1(type, max, 0)                                           \
+    MINMAX_1(type, maxnum, minmax_isnum)                             \
+    MINMAX_1(type, maxnum_noprop, minmax_isnum | minmax_snan_noprop) \
+    MINMAX_1(type, maxnummag, minmax_isnum | minmax_ismag)           \
+    MINMAX_1(type, min, minmax_ismin)                                \
+    MINMAX_1(type, minnum, minmax_ismin | minmax_isnum)              \
+    MINMAX_1(type, minnum_noprop, minmax_ismin | minmax_isnum |      \
+                                  minmax_snan_noprop)                \
     MINMAX_1(type, minnummag, minmax_ismin | minmax_isnum | minmax_ismag)
 
 MINMAX_2(float16)
diff --git a/include/fpu/softfloat.h b/include/fpu/softfloat.h
index ec7dca09606..b77917ea661 100644
--- a/include/fpu/softfloat.h
+++ b/include/fpu/softfloat.h
@@ -241,6 +241,8 @@ float16 float16_min(float16, float16, float_status *status);
 float16 float16_max(float16, float16, float_status *status);
 float16 float16_minnum(float16, float16, float_status *status);
 float16 float16_maxnum(float16, float16, float_status *status);
+float16 float16_minnum_noprop(float16, float16, float_status *status);
+float16 float16_maxnum_noprop(float16, float16, float_status *status);
 float16 float16_minnummag(float16, float16, float_status *status);
 float16 float16_maxnummag(float16, float16, float_status *status);
 float16 float16_sqrt(float16, float_status *status);
@@ -420,6 +422,8 @@ bfloat16 bfloat16_min(bfloat16, bfloat16, float_status *status);
 bfloat16 bfloat16_max(bfloat16, bfloat16, float_status *status);
 bfloat16 bfloat16_minnum(bfloat16, bfloat16, float_status *status);
 bfloat16 bfloat16_maxnum(bfloat16, bfloat16, float_status *status);
+bfloat16 bfloat16_minnum_noprop(bfloat16, bfloat16, float_status *status);
+bfloat16 bfloat16_maxnum_noprop(bfloat16, bfloat16, float_status *status);
 bfloat16 bfloat16_minnummag(bfloat16, bfloat16, float_status *status);
 bfloat16 bfloat16_maxnummag(bfloat16, bfloat16, float_status *status);
 bfloat16 bfloat16_sqrt(bfloat16, float_status *status);
@@ -587,6 +591,8 @@ float32 float32_min(float32, float32, float_status *status);
 float32 float32_max(float32, float32, float_status *status);
 float32 float32_minnum(float32, float32, float_status *status);
 float32 float32_maxnum(float32, float32, float_status *status);
+float32 float32_minnum_noprop(float32, float32, float_status *status);
+float32 float32_maxnum_noprop(float32, float32, float_status *status);
 float32 float32_minnummag(float32, float32, float_status *status);
 float32 float32_maxnummag(float32, float32, float_status *status);
 bool float32_is_quiet_nan(float32, float_status *status);
@@ -776,6 +782,8 @@ float64 float64_min(float64, float64, float_status *status);
 float64 float64_max(float64, float64, float_status *status);
 float64 float64_minnum(float64, float64, float_status *status);
 float64 float64_maxnum(float64, float64, float_status *status);
+float64 float64_minnum_noprop(float64, float64, float_status *status);
+float64 float64_maxnum_noprop(float64, float64, float_status *status);
 float64 float64_minnummag(float64, float64, float_status *status);
 float64 float64_maxnummag(float64, float64, float_status *status);
 bool float64_is_quiet_nan(float64 a, float_status *status);
@@ -1208,6 +1216,8 @@ float128 float128_min(float128, float128, float_status *status);
 float128 float128_max(float128, float128, float_status *status);
 float128 float128_minnum(float128, float128, float_status *status);
 float128 float128_maxnum(float128, float128, float_status *status);
+float128 float128_minnum_noprop(float128, float128, float_status *status);
+float128 float128_maxnum_noprop(float128, float128, float_status *status);
 float128 float128_minnummag(float128, float128, float_status *status);
 float128 float128_maxnummag(float128, float128, float_status *status);
 bool float128_is_quiet_nan(float128, float_status *status);
-- 
2.25.1

[PATCH v3 2/2] target/riscv: change the api for single/double fmin/fmax

[frank.chang]

From: Chih-Min Chao <chihmin.chao@sifive.com>

The sNaN propagation behavior has been changed since
cd20cee7 in https://github.com/riscv/riscv-isa-manual

Signed-off-by: Chih-Min Chao <chihmin.chao@sifive.com>
---
 target/riscv/fpu_helper.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/target/riscv/fpu_helper.c b/target/riscv/fpu_helper.c
index 8700516a14c..1472ead2528 100644
--- a/target/riscv/fpu_helper.c
+++ b/target/riscv/fpu_helper.c
@@ -174,14 +174,14 @@ uint64_t helper_fmin_s(CPURISCVState *env, uint64_t rs1, uint64_t rs2)
 {
     float32 frs1 = check_nanbox_s(rs1);
     float32 frs2 = check_nanbox_s(rs2);
-    return nanbox_s(float32_minnum(frs1, frs2, &env->fp_status));
+    return nanbox_s(float32_minnum_noprop(frs1, frs2, &env->fp_status));
 }
 
 uint64_t helper_fmax_s(CPURISCVState *env, uint64_t rs1, uint64_t rs2)
 {
     float32 frs1 = check_nanbox_s(rs1);
     float32 frs2 = check_nanbox_s(rs2);
-    return nanbox_s(float32_maxnum(frs1, frs2, &env->fp_status));
+    return nanbox_s(float32_maxnum_noprop(frs1, frs2, &env->fp_status));
 }
 
 uint64_t helper_fsqrt_s(CPURISCVState *env, uint64_t rs1)
@@ -283,12 +283,12 @@ uint64_t helper_fdiv_d(CPURISCVState *env, uint64_t frs1, uint64_t frs2)
 
 uint64_t helper_fmin_d(CPURISCVState *env, uint64_t frs1, uint64_t frs2)
 {
-    return float64_minnum(frs1, frs2, &env->fp_status);
+    return float64_minnum_noprop(frs1, frs2, &env->fp_status);
 }
 
 uint64_t helper_fmax_d(CPURISCVState *env, uint64_t frs1, uint64_t frs2)
 {
-    return float64_maxnum(frs1, frs2, &env->fp_status);
+    return float64_maxnum_noprop(frs1, frs2, &env->fp_status);
 }
 
 uint64_t helper_fcvt_s_d(CPURISCVState *env, uint64_t rs1)
-- 
2.25.1

Re: [PATCH v3 1/2] softfloat: add APIs to handle alternative sNaN propagation for fmax/fmin

[Richard Henderson]

On 10/14/21 11:54 PM, frank.chang@sifive.com wrote:
> +        /*
> +         * In IEEE 754-2019, minNum, maxNum, minNumMag and maxNumMag
> +         * are removed and replaced with minimum, minimumNumber, maximum
> +         * and maximumNumber.
> +         * minimumNumber/maximumNumber behavior for SNaN is changed to:
> +         *   If both operands are NaNs, a QNaN is returned.
> +         *   If either operand is a SNaN,
> +         *   an invalid operation exception is signaled,
> +         *   but unless both operands are NaNs,
> +         *   the SNaN is otherwise ignored and not converted to a QNaN.
> +         */
> +        if (!(~flags & (minmax_isnum | minmax_snan_noprop))
> +            && (ab_mask & float_cmask_snan)
> +            && (ab_mask & ~float_cmask_anynan)) {
> +            float_raise(float_flag_invalid, s);
> +            return is_nan(a->cls) ? b : a;
> +        }

This part looks ok.

> +    MINMAX_1(type, maxnum_noprop, minmax_isnum | minmax_snan_noprop) \
> +    MINMAX_1(type, minnum_noprop, minmax_ismin | minmax_isnum |      \
> +                                  minmax_snan_noprop)                \

But here, you have been given names by 754-2019: minimumNumber, maximumNumber, so I think 
you should use them.


r~

Re: [PATCH v3 2/2] target/riscv: change the api for single/double fmin/fmax

[Richard Henderson]

On 10/14/21 11:54 PM, frank.chang@sifive.com wrote:
> From: Chih-Min Chao<chihmin.chao@sifive.com>
> 
> The sNaN propagation behavior has been changed since
> cd20cee7 inhttps://github.com/riscv/riscv-isa-manual
> 
> Signed-off-by: Chih-Min Chao<chihmin.chao@sifive.com>
> ---
>   target/riscv/fpu_helper.c | 8 ++++----
>   1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/target/riscv/fpu_helper.c b/target/riscv/fpu_helper.c
> index 8700516a14c..1472ead2528 100644
> --- a/target/riscv/fpu_helper.c
> +++ b/target/riscv/fpu_helper.c
> @@ -174,14 +174,14 @@ uint64_t helper_fmin_s(CPURISCVState *env, uint64_t rs1, uint64_t rs2)
>   {
>       float32 frs1 = check_nanbox_s(rs1);
>       float32 frs2 = check_nanbox_s(rs2);
> -    return nanbox_s(float32_minnum(frs1, frs2, &env->fp_status));
> +    return nanbox_s(float32_minnum_noprop(frs1, frs2, &env->fp_status));
>   }

Don't you need to conditionalize behaviour on the isa revision?


r~

Re: [PATCH v3 1/2] softfloat: add APIs to handle alternative sNaN propagation for fmax/fmin

[Frank Chang]

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

On Sat, Oct 16, 2021 at 1:00 AM Richard Henderson <
richard.henderson@linaro.org> wrote:

> On 10/14/21 11:54 PM, frank.chang@sifive.com wrote:
> > +        /*
> > +         * In IEEE 754-2019, minNum, maxNum, minNumMag and maxNumMag
> > +         * are removed and replaced with minimum, minimumNumber, maximum
> > +         * and maximumNumber.
> > +         * minimumNumber/maximumNumber behavior for SNaN is changed to:
> > +         *   If both operands are NaNs, a QNaN is returned.
> > +         *   If either operand is a SNaN,
> > +         *   an invalid operation exception is signaled,
> > +         *   but unless both operands are NaNs,
> > +         *   the SNaN is otherwise ignored and not converted to a QNaN.
> > +         */
> > +        if (!(~flags & (minmax_isnum | minmax_snan_noprop))
> > +            && (ab_mask & float_cmask_snan)
> > +            && (ab_mask & ~float_cmask_anynan)) {
> > +            float_raise(float_flag_invalid, s);
> > +            return is_nan(a->cls) ? b : a;
> > +        }
>
> This part looks ok.
>
> > +    MINMAX_1(type, maxnum_noprop, minmax_isnum | minmax_snan_noprop) \
> > +    MINMAX_1(type, minnum_noprop, minmax_ismin | minmax_isnum |      \
> > +                                  minmax_snan_noprop)                \
>
> But here, you have been given names by 754-2019: minimumNumber,
> maximumNumber, so I think
> you should use them.
>
>
Agree, that's better than *_noprop().
Will update in my next patchset.

Thanks,
Frank Chang


>
> r~
>

[-- Attachment #2: Type: text/html, Size: 2367 bytes --]

Re: [PATCH v3 2/2] target/riscv: change the api for single/double fmin/fmax

[Frank Chang]

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

On Sat, Oct 16, 2021 at 1:05 AM Richard Henderson <
richard.henderson@linaro.org> wrote:

> On 10/14/21 11:54 PM, frank.chang@sifive.com wrote:
> > From: Chih-Min Chao<chihmin.chao@sifive.com>
> >
> > The sNaN propagation behavior has been changed since
> > cd20cee7 inhttps://github.com/riscv/riscv-isa-manual
> >
> > Signed-off-by: Chih-Min Chao<chihmin.chao@sifive.com>
> > ---
> >   target/riscv/fpu_helper.c | 8 ++++----
> >   1 file changed, 4 insertions(+), 4 deletions(-)
> >
> > diff --git a/target/riscv/fpu_helper.c b/target/riscv/fpu_helper.c
> > index 8700516a14c..1472ead2528 100644
> > --- a/target/riscv/fpu_helper.c
> > +++ b/target/riscv/fpu_helper.c
> > @@ -174,14 +174,14 @@ uint64_t helper_fmin_s(CPURISCVState *env,
> uint64_t rs1, uint64_t rs2)
> >   {
> >       float32 frs1 = check_nanbox_s(rs1);
> >       float32 frs2 = check_nanbox_s(rs2);
> > -    return nanbox_s(float32_minnum(frs1, frs2, &env->fp_status));
> > +    return nanbox_s(float32_minnum_noprop(frs1, frs2, &env->fp_status));
> >   }
>
> Don't you need to conditionalize behaviour on the isa revision?
>
>
I will pick the right API based on CPU privilege spec version.

Thanks,
Frank Chang


>
> r~
>

[-- Attachment #2: Type: text/html, Size: 2126 bytes --]

Re: [PATCH v3 2/2] target/riscv: change the api for single/double fmin/fmax

[Richard Henderson]

On 10/16/21 1:52 AM, Frank Chang wrote:
> On Sat, Oct 16, 2021 at 1:05 AM Richard Henderson <richard.henderson@linaro.org 
> <mailto:richard.henderson@linaro.org>> wrote:
> 
>     On 10/14/21 11:54 PM, frank.chang@sifive.com <mailto:frank.chang@sifive.com> wrote:
>      > From: Chih-Min Chao<chihmin.chao@sifive.com <mailto:chihmin.chao@sifive.com>>
>      >
>      > The sNaN propagation behavior has been changed since
>      > cd20cee7 inhttps://github.com/riscv/riscv-isa-manual
>     <http://github.com/riscv/riscv-isa-manual>
>      >
>      > Signed-off-by: Chih-Min Chao<chihmin.chao@sifive.com <mailto:chihmin.chao@sifive.com>>
>      > ---
>      >   target/riscv/fpu_helper.c | 8 ++++----
>      >   1 file changed, 4 insertions(+), 4 deletions(-)
>      >
>      > diff --git a/target/riscv/fpu_helper.c b/target/riscv/fpu_helper.c
>      > index 8700516a14c..1472ead2528 100644
>      > --- a/target/riscv/fpu_helper.c
>      > +++ b/target/riscv/fpu_helper.c
>      > @@ -174,14 +174,14 @@ uint64_t helper_fmin_s(CPURISCVState *env, uint64_t rs1,
>     uint64_t rs2)
>      >   {
>      >       float32 frs1 = check_nanbox_s(rs1);
>      >       float32 frs2 = check_nanbox_s(rs2);
>      > -    return nanbox_s(float32_minnum(frs1, frs2, &env->fp_status));
>      > +    return nanbox_s(float32_minnum_noprop(frs1, frs2, &env->fp_status));
>      >   }
> 
>     Don't you need to conditionalize behaviour on the isa revision?
> 
> 
> I will pick the right API based on CPU privilege spec version.

There's a separate F-extension revision number: 2.2.

But I'll leave it up to those more knowledgeable about the revision combinations actually 
present in the field to decide.


r~

Re: [PATCH v3 2/2] target/riscv: change the api for single/double fmin/fmax

[Frank Chang]

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

On Sun, Oct 17, 2021 at 1:56 AM Richard Henderson <
richard.henderson@linaro.org> wrote:

> On 10/16/21 1:52 AM, Frank Chang wrote:
> > On Sat, Oct 16, 2021 at 1:05 AM Richard Henderson <
> richard.henderson@linaro.org
> > <mailto:richard.henderson@linaro.org>> wrote:
> >
> >     On 10/14/21 11:54 PM, frank.chang@sifive.com <mailto:
> frank.chang@sifive.com> wrote:
> >      > From: Chih-Min Chao<chihmin.chao@sifive.com <mailto:
> chihmin.chao@sifive.com>>
> >      >
> >      > The sNaN propagation behavior has been changed since
> >      > cd20cee7 inhttps://github.com/riscv/riscv-isa-manual
> >     <http://github.com/riscv/riscv-isa-manual>
> >      >
> >      > Signed-off-by: Chih-Min Chao<chihmin.chao@sifive.com <mailto:
> chihmin.chao@sifive.com>>
> >      > ---
> >      >   target/riscv/fpu_helper.c | 8 ++++----
> >      >   1 file changed, 4 insertions(+), 4 deletions(-)
> >      >
> >      > diff --git a/target/riscv/fpu_helper.c b/target/riscv/fpu_helper.c
> >      > index 8700516a14c..1472ead2528 100644
> >      > --- a/target/riscv/fpu_helper.c
> >      > +++ b/target/riscv/fpu_helper.c
> >      > @@ -174,14 +174,14 @@ uint64_t helper_fmin_s(CPURISCVState *env,
> uint64_t rs1,
> >     uint64_t rs2)
> >      >   {
> >      >       float32 frs1 = check_nanbox_s(rs1);
> >      >       float32 frs2 = check_nanbox_s(rs2);
> >      > -    return nanbox_s(float32_minnum(frs1, frs2, &env->fp_status));
> >      > +    return nanbox_s(float32_minnum_noprop(frs1, frs2,
> &env->fp_status));
> >      >   }
> >
> >     Don't you need to conditionalize behaviour on the isa revision?
> >
> >
> > I will pick the right API based on CPU privilege spec version.
>
> There's a separate F-extension revision number: 2.2.
>
> But I'll leave it up to those more knowledgeable about the revision
> combinations actually
> present in the field to decide.
>
>
I did some history searches on RISC-V ISA spec Github repo.

F-extension was bumped to v2.2 at (2018/08/28):
https://github.com/riscv/riscv-isa-manual/releases/tag/draft-20180828-eb78171
The privilege spec is v1.10-draft at the time.

and later ratified at (2019/03/26):
https://github.com/riscv/riscv-isa-manual/releases/tag/IMFDQC-Ratification-20190305

The spec was updated to use IEEE 754-2019 min/max functions in commit:
#cd20cee7
<https://github.com/riscv/riscv-isa-manual/commit/cd20cee7efd9bac7c5aa127ec3b451749d2b3cce>
 (2019/06/05).

Privilege spec v1.11 is ratified at (2019/06/10):
https://github.com/riscv/riscv-isa-manual/releases/tag/Ratified-IMFDQC-and-Priv-v1.11

In fact, Unprivileged spec v2.2 was released at (2017/05/10):
https://github.com/riscv/riscv-isa-manual/releases/tag/riscv-user-2.2

and Privilege spec v1.10 was released at (2017/05/10):
https://github.com/riscv/riscv-isa-manual/releases/tag/riscv-priv-1.10

Privilege spec was then bumped to v1.11-draft in the next draft release
right after v1.10 (2018/05/24):
https://github.com/riscv/riscv-isa-manual/releases/tag/draft-20180524001518-9981ad7
(RVF was still v2.0 at the time.)

It seems that when Privilege spec v1.11 was ratified, RVF had been bumped
to v2.2,
and when Privilege spec v1.10 was ratified, RVF was still v2.0.

As in QEMU, there's only *priv_ver* variable existing for now.
So unless we introduce other variables like: *unpriv_ver* or *fext_ver*.
Otherwise, I think using *priv_ver* is still valid here.
Though it is not accurate, somehow confused,
and may not be true anymore in future standards.

Let me know which way is better for our maintenance.

Thanks,
Frank Chang

r~
>

[-- Attachment #2: Type: text/html, Size: 6137 bytes --]

Re: [PATCH v3 2/2] target/riscv: change the api for single/double fmin/fmax

[Frank Chang]

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

On Sun, Oct 17, 2021 at 8:55 AM Frank Chang <frank.chang@sifive.com> wrote:

> On Sun, Oct 17, 2021 at 1:56 AM Richard Henderson <
> richard.henderson@linaro.org> wrote:
>
>> On 10/16/21 1:52 AM, Frank Chang wrote:
>> > On Sat, Oct 16, 2021 at 1:05 AM Richard Henderson <
>> richard.henderson@linaro.org
>> > <mailto:richard.henderson@linaro.org>> wrote:
>> >
>> >     On 10/14/21 11:54 PM, frank.chang@sifive.com <mailto:
>> frank.chang@sifive.com> wrote:
>> >      > From: Chih-Min Chao<chihmin.chao@sifive.com <mailto:
>> chihmin.chao@sifive.com>>
>> >      >
>> >      > The sNaN propagation behavior has been changed since
>> >      > cd20cee7 inhttps://github.com/riscv/riscv-isa-manual
>> >     <http://github.com/riscv/riscv-isa-manual>
>> >      >
>> >      > Signed-off-by: Chih-Min Chao<chihmin.chao@sifive.com <mailto:
>> chihmin.chao@sifive.com>>
>> >      > ---
>> >      >   target/riscv/fpu_helper.c | 8 ++++----
>> >      >   1 file changed, 4 insertions(+), 4 deletions(-)
>> >      >
>> >      > diff --git a/target/riscv/fpu_helper.c
>> b/target/riscv/fpu_helper.c
>> >      > index 8700516a14c..1472ead2528 100644
>> >      > --- a/target/riscv/fpu_helper.c
>> >      > +++ b/target/riscv/fpu_helper.c
>> >      > @@ -174,14 +174,14 @@ uint64_t helper_fmin_s(CPURISCVState *env,
>> uint64_t rs1,
>> >     uint64_t rs2)
>> >      >   {
>> >      >       float32 frs1 = check_nanbox_s(rs1);
>> >      >       float32 frs2 = check_nanbox_s(rs2);
>> >      > -    return nanbox_s(float32_minnum(frs1, frs2,
>> &env->fp_status));
>> >      > +    return nanbox_s(float32_minnum_noprop(frs1, frs2,
>> &env->fp_status));
>> >      >   }
>> >
>> >     Don't you need to conditionalize behaviour on the isa revision?
>> >
>> >
>> > I will pick the right API based on CPU privilege spec version.
>>
>> There's a separate F-extension revision number: 2.2.
>>
>> But I'll leave it up to those more knowledgeable about the revision
>> combinations actually
>> present in the field to decide.
>>
>>
> I did some history searches on RISC-V ISA spec Github repo.
>
> F-extension was bumped to v2.2 at (2018/08/28):
>
> https://github.com/riscv/riscv-isa-manual/releases/tag/draft-20180828-eb78171
> The privilege spec is v1.10-draft at the time.
>
> and later ratified at (2019/03/26):
>
> https://github.com/riscv/riscv-isa-manual/releases/tag/IMFDQC-Ratification-20190305
>
> The spec was updated to use IEEE 754-2019 min/max functions in commit:
> #cd20cee7
> <https://github.com/riscv/riscv-isa-manual/commit/cd20cee7efd9bac7c5aa127ec3b451749d2b3cce>
>  (2019/06/05).
>

Sorry, the commit date is 2017/06/05, not 2019/06/05.

But I think it's probably easier and clearer to just introduce an extra
*fext_ver* variable.
We can set CPUs which are Privilege spec v1.10 to RVF v2.0
(FEXT_VERSION_2_00_0),
and others with Privilege spec v1.11 to RVF v2.2 (FEXT_VERSION_2_02_0).

Any comments are welcome.

Regards,
Frank Chang


>
> Privilege spec v1.11 is ratified at (2019/06/10):
>
> https://github.com/riscv/riscv-isa-manual/releases/tag/Ratified-IMFDQC-and-Priv-v1.11
>
> In fact, Unprivileged spec v2.2 was released at (2017/05/10):
> https://github.com/riscv/riscv-isa-manual/releases/tag/riscv-user-2.2
>
> and Privilege spec v1.10 was released at (2017/05/10):
> https://github.com/riscv/riscv-isa-manual/releases/tag/riscv-priv-1.10
>
> Privilege spec was then bumped to v1.11-draft in the next draft release
> right after v1.10 (2018/05/24):
>
> https://github.com/riscv/riscv-isa-manual/releases/tag/draft-20180524001518-9981ad7
> (RVF was still v2.0 at the time.)
>
> It seems that when Privilege spec v1.11 was ratified, RVF had been bumped
> to v2.2,
> and when Privilege spec v1.10 was ratified, RVF was still v2.0.
>
> As in QEMU, there's only *priv_ver* variable existing for now.
> So unless we introduce other variables like: *unpriv_ver* or *fext_ver*.
> Otherwise, I think using *priv_ver* is still valid here.
> Though it is not accurate, somehow confused,
> and may not be true anymore in future standards.
>
> Let me know which way is better for our maintenance.
>
> Thanks,
> Frank Chang
>
> r~
>>
>

[-- Attachment #2: Type: text/html, Size: 7294 bytes --]

Re: [PATCH v3 2/2] target/riscv: change the api for single/double fmin/fmax

[Alistair Francis]

On Sun, Oct 17, 2021 at 4:59 PM Frank Chang <frank.chang@sifive.com> wrote:
>
> On Sun, Oct 17, 2021 at 8:55 AM Frank Chang <frank.chang@sifive.com> wrote:
>>
>> On Sun, Oct 17, 2021 at 1:56 AM Richard Henderson <richard.henderson@linaro.org> wrote:
>>>
>>> On 10/16/21 1:52 AM, Frank Chang wrote:
>>> > On Sat, Oct 16, 2021 at 1:05 AM Richard Henderson <richard.henderson@linaro.org
>>> > <mailto:richard.henderson@linaro.org>> wrote:
>>> >
>>> >     On 10/14/21 11:54 PM, frank.chang@sifive.com <mailto:frank.chang@sifive.com> wrote:
>>> >      > From: Chih-Min Chao<chihmin.chao@sifive.com <mailto:chihmin.chao@sifive.com>>
>>> >      >
>>> >      > The sNaN propagation behavior has been changed since
>>> >      > cd20cee7 inhttps://github.com/riscv/riscv-isa-manual
>>> >     <http://github.com/riscv/riscv-isa-manual>
>>> >      >
>>> >      > Signed-off-by: Chih-Min Chao<chihmin.chao@sifive.com <mailto:chihmin.chao@sifive.com>>
>>> >      > ---
>>> >      >   target/riscv/fpu_helper.c | 8 ++++----
>>> >      >   1 file changed, 4 insertions(+), 4 deletions(-)
>>> >      >
>>> >      > diff --git a/target/riscv/fpu_helper.c b/target/riscv/fpu_helper.c
>>> >      > index 8700516a14c..1472ead2528 100644
>>> >      > --- a/target/riscv/fpu_helper.c
>>> >      > +++ b/target/riscv/fpu_helper.c
>>> >      > @@ -174,14 +174,14 @@ uint64_t helper_fmin_s(CPURISCVState *env, uint64_t rs1,
>>> >     uint64_t rs2)
>>> >      >   {
>>> >      >       float32 frs1 = check_nanbox_s(rs1);
>>> >      >       float32 frs2 = check_nanbox_s(rs2);
>>> >      > -    return nanbox_s(float32_minnum(frs1, frs2, &env->fp_status));
>>> >      > +    return nanbox_s(float32_minnum_noprop(frs1, frs2, &env->fp_status));
>>> >      >   }
>>> >
>>> >     Don't you need to conditionalize behaviour on the isa revision?
>>> >
>>> >
>>> > I will pick the right API based on CPU privilege spec version.
>>>
>>> There's a separate F-extension revision number: 2.2.
>>>
>>> But I'll leave it up to those more knowledgeable about the revision combinations actually
>>> present in the field to decide.
>>>
>>
>> I did some history searches on RISC-V ISA spec Github repo.
>>
>> F-extension was bumped to v2.2 at (2018/08/28):
>> https://github.com/riscv/riscv-isa-manual/releases/tag/draft-20180828-eb78171
>> The privilege spec is v1.10-draft at the time.
>>
>> and later ratified at (2019/03/26):
>> https://github.com/riscv/riscv-isa-manual/releases/tag/IMFDQC-Ratification-20190305
>>
>> The spec was updated to use IEEE 754-2019 min/max functions in commit: #cd20cee7 (2019/06/05).
>
>
> Sorry, the commit date is 2017/06/05, not 2019/06/05.
>
> But I think it's probably easier and clearer to just introduce an extra fext_ver variable.
> We can set CPUs which are Privilege spec v1.10 to RVF v2.0 (FEXT_VERSION_2_00_0),
> and others with Privilege spec v1.11 to RVF v2.2 (FEXT_VERSION_2_02_0).

I think it's probably simpler to just tie this to the priv_spec. It's
not completely accurate, but it should be close enough. Otherwise we
have the risk of having too many version variables and it becomes a
pain for users to deal with.

If the unpriv spec is better, we could also introduce that. We will
probably need that one day for something else anyway.

If you feel that we really need a fext_ver (to avoid large software
breakage for example) then it's also ok, we just need to justify why.

Alistair

>
> Any comments are welcome.
>
> Regards,
> Frank Chang
>
>>
>>
>> Privilege spec v1.11 is ratified at (2019/06/10):
>> https://github.com/riscv/riscv-isa-manual/releases/tag/Ratified-IMFDQC-and-Priv-v1.11
>>
>> In fact, Unprivileged spec v2.2 was released at (2017/05/10):
>> https://github.com/riscv/riscv-isa-manual/releases/tag/riscv-user-2.2
>>
>> and Privilege spec v1.10 was released at (2017/05/10):
>> https://github.com/riscv/riscv-isa-manual/releases/tag/riscv-priv-1.10
>>
>> Privilege spec was then bumped to v1.11-draft in the next draft release right after v1.10 (2018/05/24):
>> https://github.com/riscv/riscv-isa-manual/releases/tag/draft-20180524001518-9981ad7
>> (RVF was still v2.0 at the time.)
>>
>> It seems that when Privilege spec v1.11 was ratified, RVF had been bumped to v2.2,
>> and when Privilege spec v1.10 was ratified, RVF was still v2.0.
>>
>> As in QEMU, there's only priv_ver variable existing for now.
>> So unless we introduce other variables like: unpriv_ver or fext_ver.
>> Otherwise, I think using priv_ver is still valid here.
>> Though it is not accurate, somehow confused,
>> and may not be true anymore in future standards.
>>
>> Let me know which way is better for our maintenance.
>>
>> Thanks,
>> Frank Chang
>>
>>> r~

Re: [PATCH v3 2/2] target/riscv: change the api for single/double fmin/fmax

[Frank Chang]

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

On Mon, Oct 18, 2021 at 8:18 AM Alistair Francis <alistair23@gmail.com>
wrote:

> On Sun, Oct 17, 2021 at 4:59 PM Frank Chang <frank.chang@sifive.com>
> wrote:
> >
> > On Sun, Oct 17, 2021 at 8:55 AM Frank Chang <frank.chang@sifive.com>
> wrote:
> >>
> >> On Sun, Oct 17, 2021 at 1:56 AM Richard Henderson <
> richard.henderson@linaro.org> wrote:
> >>>
> >>> On 10/16/21 1:52 AM, Frank Chang wrote:
> >>> > On Sat, Oct 16, 2021 at 1:05 AM Richard Henderson <
> richard.henderson@linaro.org
> >>> > <mailto:richard.henderson@linaro.org>> wrote:
> >>> >
> >>> >     On 10/14/21 11:54 PM, frank.chang@sifive.com <mailto:
> frank.chang@sifive.com> wrote:
> >>> >      > From: Chih-Min Chao<chihmin.chao@sifive.com <mailto:
> chihmin.chao@sifive.com>>
> >>> >      >
> >>> >      > The sNaN propagation behavior has been changed since
> >>> >      > cd20cee7 inhttps://github.com/riscv/riscv-isa-manual
> >>> >     <http://github.com/riscv/riscv-isa-manual>
> >>> >      >
> >>> >      > Signed-off-by: Chih-Min Chao<chihmin.chao@sifive.com <mailto:
> chihmin.chao@sifive.com>>
> >>> >      > ---
> >>> >      >   target/riscv/fpu_helper.c | 8 ++++----
> >>> >      >   1 file changed, 4 insertions(+), 4 deletions(-)
> >>> >      >
> >>> >      > diff --git a/target/riscv/fpu_helper.c
> b/target/riscv/fpu_helper.c
> >>> >      > index 8700516a14c..1472ead2528 100644
> >>> >      > --- a/target/riscv/fpu_helper.c
> >>> >      > +++ b/target/riscv/fpu_helper.c
> >>> >      > @@ -174,14 +174,14 @@ uint64_t helper_fmin_s(CPURISCVState
> *env, uint64_t rs1,
> >>> >     uint64_t rs2)
> >>> >      >   {
> >>> >      >       float32 frs1 = check_nanbox_s(rs1);
> >>> >      >       float32 frs2 = check_nanbox_s(rs2);
> >>> >      > -    return nanbox_s(float32_minnum(frs1, frs2,
> &env->fp_status));
> >>> >      > +    return nanbox_s(float32_minnum_noprop(frs1, frs2,
> &env->fp_status));
> >>> >      >   }
> >>> >
> >>> >     Don't you need to conditionalize behaviour on the isa revision?
> >>> >
> >>> >
> >>> > I will pick the right API based on CPU privilege spec version.
> >>>
> >>> There's a separate F-extension revision number: 2.2.
> >>>
> >>> But I'll leave it up to those more knowledgeable about the revision
> combinations actually
> >>> present in the field to decide.
> >>>
> >>
> >> I did some history searches on RISC-V ISA spec Github repo.
> >>
> >> F-extension was bumped to v2.2 at (2018/08/28):
> >>
> https://github.com/riscv/riscv-isa-manual/releases/tag/draft-20180828-eb78171
> >> The privilege spec is v1.10-draft at the time.
> >>
> >> and later ratified at (2019/03/26):
> >>
> https://github.com/riscv/riscv-isa-manual/releases/tag/IMFDQC-Ratification-20190305
> >>
> >> The spec was updated to use IEEE 754-2019 min/max functions in commit:
> #cd20cee7 (2019/06/05).
> >
> >
> > Sorry, the commit date is 2017/06/05, not 2019/06/05.
> >
> > But I think it's probably easier and clearer to just introduce an extra
> fext_ver variable.
> > We can set CPUs which are Privilege spec v1.10 to RVF v2.0
> (FEXT_VERSION_2_00_0),
> > and others with Privilege spec v1.11 to RVF v2.2 (FEXT_VERSION_2_02_0).
>
> I think it's probably simpler to just tie this to the priv_spec. It's
> not completely accurate, but it should be close enough. Otherwise we
> have the risk of having too many version variables and it becomes a
> pain for users to deal with.
>
> If the unpriv spec is better, we could also introduce that. We will
> probably need that one day for something else anyway.
>
> If you feel that we really need a fext_ver (to avoid large software
> breakage for example) then it's also ok, we just need to justify why.
>
> Alistair
>

A little problem with Unpriv spec is that it uses v2.2 and later with the
date as of version tag.
It shouldn't be a real problem because we can still use the date v2.2. was
released.

But if it's okay to tie RVF version with Priv spec version,
then let's just use Priv spec version for now.
We can introduce other version variables when we really need them in the
future.

Thanks,
Frank Chang


>
> >
> > Any comments are welcome.
> >
> > Regards,
> > Frank Chang
> >
> >>
> >>
> >> Privilege spec v1.11 is ratified at (2019/06/10):
> >>
> https://github.com/riscv/riscv-isa-manual/releases/tag/Ratified-IMFDQC-and-Priv-v1.11
> >>
> >> In fact, Unprivileged spec v2.2 was released at (2017/05/10):
> >> https://github.com/riscv/riscv-isa-manual/releases/tag/riscv-user-2.2
> >>
> >> and Privilege spec v1.10 was released at (2017/05/10):
> >> https://github.com/riscv/riscv-isa-manual/releases/tag/riscv-priv-1.10
> >>
> >> Privilege spec was then bumped to v1.11-draft in the next draft release
> right after v1.10 (2018/05/24):
> >>
> https://github.com/riscv/riscv-isa-manual/releases/tag/draft-20180524001518-9981ad7
> >> (RVF was still v2.0 at the time.)
> >>
> >> It seems that when Privilege spec v1.11 was ratified, RVF had been
> bumped to v2.2,
> >> and when Privilege spec v1.10 was ratified, RVF was still v2.0.
> >>
> >> As in QEMU, there's only priv_ver variable existing for now.
> >> So unless we introduce other variables like: unpriv_ver or fext_ver.
> >> Otherwise, I think using priv_ver is still valid here.
> >> Though it is not accurate, somehow confused,
> >> and may not be true anymore in future standards.
> >>
> >> Let me know which way is better for our maintenance.
> >>
> >> Thanks,
> >> Frank Chang
> >>
> >>> r~
>

[-- Attachment #2: Type: text/html, Size: 8879 bytes --]

Re: [PATCH v3 2/2] target/riscv: change the api for single/double fmin/fmax

[Frank Chang]

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

<frank.chang@sifive.com> 於 2021年10月15日 週五 下午2:12寫道：

> From: Chih-Min Chao <chihmin.chao@sifive.com>
>
> The sNaN propagation behavior has been changed since
> cd20cee7 in https://github.com/riscv/riscv-isa-manual
>
> Signed-off-by: Chih-Min Chao <chihmin.chao@sifive.com>
> ---
>  target/riscv/fpu_helper.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/target/riscv/fpu_helper.c b/target/riscv/fpu_helper.c
> index 8700516a14c..1472ead2528 100644
> --- a/target/riscv/fpu_helper.c
> +++ b/target/riscv/fpu_helper.c
> @@ -174,14 +174,14 @@ uint64_t helper_fmin_s(CPURISCVState *env, uint64_t
> rs1, uint64_t rs2)
>  {
>      float32 frs1 = check_nanbox_s(rs1);
>      float32 frs2 = check_nanbox_s(rs2);
> -    return nanbox_s(float32_minnum(frs1, frs2, &env->fp_status));
> +    return nanbox_s(float32_minnum_noprop(frs1, frs2, &env->fp_status));
>  }
>
>  uint64_t helper_fmax_s(CPURISCVState *env, uint64_t rs1, uint64_t rs2)
>  {
>      float32 frs1 = check_nanbox_s(rs1);
>      float32 frs2 = check_nanbox_s(rs2);
> -    return nanbox_s(float32_maxnum(frs1, frs2, &env->fp_status));
> +    return nanbox_s(float32_maxnum_noprop(frs1, frs2, &env->fp_status));
>  }
>
>  uint64_t helper_fsqrt_s(CPURISCVState *env, uint64_t rs1)
> @@ -283,12 +283,12 @@ uint64_t helper_fdiv_d(CPURISCVState *env, uint64_t
> frs1, uint64_t frs2)
>
>  uint64_t helper_fmin_d(CPURISCVState *env, uint64_t frs1, uint64_t frs2)
>  {
> -    return float64_minnum(frs1, frs2, &env->fp_status);
> +    return float64_minnum_noprop(frs1, frs2, &env->fp_status);
>  }
>
>  uint64_t helper_fmax_d(CPURISCVState *env, uint64_t frs1, uint64_t frs2)
>  {
> -    return float64_maxnum(frs1, frs2, &env->fp_status);
> +    return float64_maxnum_noprop(frs1, frs2, &env->fp_status);
>  }
>
>  uint64_t helper_fcvt_s_d(CPURISCVState *env, uint64_t rs1)
> --
> 2.25.1
>
>
>
I should add the cover letter for this series of patchset, will resend it.
Sorry for the confusion.

Regards,
Frank Chang

[-- Attachment #2: Type: text/html, Size: 2829 bytes --]

[PATCH v3 2/2] target/riscv: change the api for single/double fmin/fmax

[frank.chang]

From: Chih-Min Chao <chihmin.chao@sifive.com>

The sNaN propagation behavior has been changed since
cd20cee7 in https://github.com/riscv/riscv-isa-manual

Signed-off-by: Chih-Min Chao <chihmin.chao@sifive.com>
---
 target/riscv/fpu_helper.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/target/riscv/fpu_helper.c b/target/riscv/fpu_helper.c
index 8700516a14c..1472ead2528 100644
--- a/target/riscv/fpu_helper.c
+++ b/target/riscv/fpu_helper.c
@@ -174,14 +174,14 @@ uint64_t helper_fmin_s(CPURISCVState *env, uint64_t rs1, uint64_t rs2)
 {
     float32 frs1 = check_nanbox_s(rs1);
     float32 frs2 = check_nanbox_s(rs2);
-    return nanbox_s(float32_minnum(frs1, frs2, &env->fp_status));
+    return nanbox_s(float32_minnum_noprop(frs1, frs2, &env->fp_status));
 }
 
 uint64_t helper_fmax_s(CPURISCVState *env, uint64_t rs1, uint64_t rs2)
 {
     float32 frs1 = check_nanbox_s(rs1);
     float32 frs2 = check_nanbox_s(rs2);
-    return nanbox_s(float32_maxnum(frs1, frs2, &env->fp_status));
+    return nanbox_s(float32_maxnum_noprop(frs1, frs2, &env->fp_status));
 }
 
 uint64_t helper_fsqrt_s(CPURISCVState *env, uint64_t rs1)
@@ -283,12 +283,12 @@ uint64_t helper_fdiv_d(CPURISCVState *env, uint64_t frs1, uint64_t frs2)
 
 uint64_t helper_fmin_d(CPURISCVState *env, uint64_t frs1, uint64_t frs2)
 {
-    return float64_minnum(frs1, frs2, &env->fp_status);
+    return float64_minnum_noprop(frs1, frs2, &env->fp_status);
 }
 
 uint64_t helper_fmax_d(CPURISCVState *env, uint64_t frs1, uint64_t frs2)
 {
-    return float64_maxnum(frs1, frs2, &env->fp_status);
+    return float64_maxnum_noprop(frs1, frs2, &env->fp_status);
 }
 
 uint64_t helper_fcvt_s_d(CPURISCVState *env, uint64_t rs1)
-- 
2.25.1