Re: [RFC 62/65] fpu: add api to handle alternative sNaN propagation

[Alex Bennée]

frank.chang@sifive.com writes:

> From: Chih-Min Chao <chihmin.chao@sifive.com>
>
> Signed-off-by: Chih-Min Chao <chihmin.chao@sifive.com>
> Signed-off-by: Frank Chang <frank.chang@sifive.com>
> ---
>  fpu/softfloat.c         | 68 +++++++++++++++++++++++++----------------
>  include/fpu/softfloat.h |  6 ++++
>  2 files changed, 48 insertions(+), 26 deletions(-)
>
> diff --git a/fpu/softfloat.c b/fpu/softfloat.c
> index fa1c99c03e..028b857167 100644
> --- a/fpu/softfloat.c
> +++ b/fpu/softfloat.c
> @@ -2777,23 +2777,32 @@ float64 uint16_to_float64(uint16_t a, float_status 
> *status)
>   * and minNumMag() from the IEEE-754 2008.
>   */
>  static FloatParts minmax_floats(FloatParts a, FloatParts b, bool ismin,
> -                                bool ieee, bool ismag, float_status *s)
> +                                bool ieee, bool ismag, bool issnan_prop,
> +                                float_status *s)
>  {
>      if (unlikely(is_nan(a.cls) || is_nan(b.cls))) {
>          if (ieee) {
>              /* Takes two floating-point values `a' and `b', one of
>               * which is a NaN, and returns the appropriate NaN
>               * result. If either `a' or `b' is a signaling NaN,
> -             * the invalid exception is raised.
> +             * the invalid exception is raised but the NaN
> +             * propagation is 'shall'.
>               */
>              if (is_snan(a.cls) || is_snan(b.cls)) {
> -                return pick_nan(a, b, s);
> -            } else if (is_nan(a.cls) && !is_nan(b.cls)) {
> +                if (issnan_prop) {
> +                    pick_nan(a, b, s);

This looks funky to me because you don't actually pick a nan - are you
just using this for side effects?

I'm also confused by the fact the new helpers have the prototype noprop
which implies no propagation yes the bool flag is true and named
issnan_prop which implies it should propagate.

I think we need a clearer problem statement in the commit of what you
are trying to achieve here. I suspect it might be worth splitting the
flag setting from pick_nan to it's own mini helper if that is all we
want to do in this case.

> +                } else {
> +                    return pick_nan(a, b, s);
> +                }
> +            }
> +
> +            if (is_nan(a.cls) && !is_nan(b.cls)) {
>                  return b;
>              } else if (is_nan(b.cls) && !is_nan(a.cls)) {
>                  return a;
>              }
>          }
> +

nit: stray space

>          return pick_nan(a, b, s);
>      } else {
>          int a_exp, b_exp;
> @@ -2847,37 +2856,44 @@ static FloatParts minmax_floats(FloatParts a, 
> FloatParts b, bool ismin,
>      }
>  }
>  
> -#define MINMAX(sz, name, ismin, isiee, ismag)                           \
> +#define MINMAX(sz, name, ismin, isiee, ismag, issnan_prop)              \
>  float ## sz float ## sz ## _ ## name(float ## sz a, float ## sz b,      \
>                                       float_status *s)                   \
>  {                                                                       \
>      FloatParts pa = float ## sz ## _unpack_canonical(a, s);             \
>      FloatParts pb = float ## sz ## _unpack_canonical(b, s);             \
> -    FloatParts pr = minmax_floats(pa, pb, ismin, isiee, ismag, s);      \
> +    FloatParts pr = minmax_floats(pa, pb, ismin, isiee, ismag,          \
> +                                  issnan_prop, s);                      \
>                                                                          \
>      return float ## sz ## _round_pack_canonical(pr, s);                 \
>  }
>  
> -MINMAX(16, min, true, false, false)
> -MINMAX(16, minnum, true, true, false)
> -MINMAX(16, minnummag, true, true, true)
> -MINMAX(16, max, false, false, false)
> -MINMAX(16, maxnum, false, true, false)
> -MINMAX(16, maxnummag, false, true, true)
> -
> -MINMAX(32, min, true, false, false)
> -MINMAX(32, minnum, true, true, false)
> -MINMAX(32, minnummag, true, true, true)
> -MINMAX(32, max, false, false, false)
> -MINMAX(32, maxnum, false, true, false)
> -MINMAX(32, maxnummag, false, true, true)
> -
> -MINMAX(64, min, true, false, false)
> -MINMAX(64, minnum, true, true, false)
> -MINMAX(64, minnummag, true, true, true)
> -MINMAX(64, max, false, false, false)
> -MINMAX(64, maxnum, false, true, false)
> -MINMAX(64, maxnummag, false, true, true)
> +MINMAX(16, min, true, false, false, false)
> +MINMAX(16, minnum, true, true, false, false)
> +MINMAX(16, minnum_noprop, true, true, false, true)
> +MINMAX(16, minnummag, true, true, true, false)
> +MINMAX(16, max, false, false, false, false)
> +MINMAX(16, maxnum, false, true, false, false)
> +MINMAX(16, maxnum_noprop, false, true, false, true)
> +MINMAX(16, maxnummag, false, true, true, false)
> +
> +MINMAX(32, min, true, false, false, false)
> +MINMAX(32, minnum, true, true, false, false)
> +MINMAX(32, minnum_noprop, true, true, false, true)
> +MINMAX(32, minnummag, true, true, true, false)
> +MINMAX(32, max, false, false, false, false)
> +MINMAX(32, maxnum, false, true, false, false)
> +MINMAX(32, maxnum_noprop, false, true, false, true)
> +MINMAX(32, maxnummag, false, true, true, false)
> +
> +MINMAX(64, min, true, false, false, false)
> +MINMAX(64, minnum, true, true, false, false)
> +MINMAX(64, minnum_noprop, true, true, false, true)
> +MINMAX(64, minnummag, true, true, true, false)
> +MINMAX(64, max, false, false, false, false)
> +MINMAX(64, maxnum, false, true, false, false)
> +MINMAX(64, maxnum_noprop, false, true, false, true)
> +MINMAX(64, maxnummag, false, true, true, false)
>  
>  #undef MINMAX
>  
> diff --git a/include/fpu/softfloat.h b/include/fpu/softfloat.h
> index b0ae8f6295..075c680456 100644
> --- a/include/fpu/softfloat.h
> +++ b/include/fpu/softfloat.h
> @@ -239,6 +239,8 @@ float16 float16_minnum(float16, float16, float_status 
> *status);
>  float16 float16_maxnum(float16, float16, float_status *status);
>  float16 float16_minnummag(float16, float16, float_status *status);
>  float16 float16_maxnummag(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_sqrt(float16, float_status *status);
>  FloatRelation float16_compare(float16, float16, float_status *status);
>  FloatRelation float16_compare_quiet(float16, float16, float_status *status);
> @@ -359,6 +361,8 @@ float32 float32_minnum(float32, float32, float_status 
> *status);
>  float32 float32_maxnum(float32, float32, float_status *status);
>  float32 float32_minnummag(float32, float32, float_status *status);
>  float32 float32_maxnummag(float32, float32, float_status *status);
> +float32 float32_minnum_noprop(float32, float32, float_status *status);
> +float32 float32_maxnum_noprop(float32, float32, float_status *status);
>  bool float32_is_quiet_nan(float32, float_status *status);
>  bool float32_is_signaling_nan(float32, float_status *status);
>  float32 float32_silence_nan(float32, float_status *status);
> @@ -548,6 +552,8 @@ float64 float64_minnum(float64, float64, float_status 
> *status);
>  float64 float64_maxnum(float64, float64, float_status *status);
>  float64 float64_minnummag(float64, float64, float_status *status);
>  float64 float64_maxnummag(float64, float64, float_status *status);
> +float64 float64_minnum_noprop(float64, float64, float_status *status);
> +float64 float64_maxnum_noprop(float64, float64, float_status *status);
>  bool float64_is_quiet_nan(float64 a, float_status *status);
>  bool float64_is_signaling_nan(float64, float_status *status);
>  float64 float64_silence_nan(float64, float_status *status);


-- 
Alex Bennée