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
Hi Alex,
1.
This patch comes from the change of sNaN propagation implementation of riscv floating spec.
Take following as example,
fmin.s ft0, ft1, ft2.
For spec 2.2, the sNaN handling for fmin and fmax is
if ft1 is sNaN or ft2 is sNaN
a. set the invalid flag
b. ft0 is canonical NaN
ref:
https://github.com/riscv/riscv-isa-manual/releases/tag/riscv-user-2.2 section 8.3
"For FMIN and FMAX, if at least one input is a signaling NaN, or if both inputs are quiet NaNs,
the result is the canonical NaN. If one operand is a quiet NaN and the other is not a NaN, the
result is the non-NaN operand.
For spec 20191213, the behavior is changed to
if ft1 or ft2 is sNaN and the other is non-NaN
a. set the invalid flag
b. ft0 is set to non-NaN source
ref:
https://github.com/riscv/riscv-isa-manual/releases/tag/Ratified-IMAFDQC section 11.6
"If both inputs are NaNs, the result is
the canonical NaN. If only one operand is a NaN, the result is the non-NaN operand. Signaling
NaN inputs set the invalid operation exception flag, even when the result is not NaN."
2.
As you guess, the patch takes the side effect of pick_nan. The pick_nan does two works
a. set invalid flag if input is sNaN
b. return correct NaN number by configuration
for one possible case, one operand is sNaN and the other is non-NaN, the patch does
a. pick_nan to set invalid_flag but doesn't use the return value)
b. return non-NaN
for the other case, both operands are sNaN, the patch does
a. pick_nan to set_invalid_flag
b pick_nan to return NaN value
Is it better to separate the "set invalid flag" part from pick_nan to make it concrete ?
3.
The parameter naming is misleading and will be fix in next separated softfloat patch.
Thanks
Chih-Min Chao