From: Chih-Min Chao <chihmin.chao@sifive.com>
To: "Alex Bennée" <alex.bennee@linaro.org>
Cc: frank.chang@sifive.com, Peter Maydell <peter.maydell@linaro.org>,
"open list:RISC-V" <qemu-riscv@nongnu.org>,
"qemu-devel@nongnu.org Developers" <qemu-devel@nongnu.org>,
Aurelien Jarno <aurelien@aurel32.net>
Subject: Re: [RFC 62/65] fpu: add api to handle alternative sNaN propagation
Date: Tue, 14 Jul 2020 01:38:22 +0800 [thread overview]
Message-ID: <CAEiOBXXY33gxpBBmu0N8vauqFNXdnbWJiA58nUtHvRBtYFe-EQ@mail.gmail.com> (raw)
In-Reply-To: <87h7uflg65.fsf@linaro.org>
[-- Attachment #1: Type: text/plain, Size: 10163 bytes --]
On Fri, Jul 10, 2020 at 8:15 PM Alex Bennée <alex.bennee@linaro.org> wrote:
>
> 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
[-- Attachment #2: Type: text/html, Size: 13217 bytes --]
next prev parent reply other threads:[~2020-07-13 17:39 UTC|newest]
Thread overview: 107+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-07-10 10:48 [RFC 00/65] target/riscv: support vector extension v0.9 frank.chang
2020-07-10 10:48 ` [RFC 01/65] target/riscv: fix rsub gvec tcg_assert_listed_vecop assertion frank.chang
2020-07-10 16:12 ` Richard Henderson
2020-07-10 18:24 ` Alistair Francis
2020-07-10 10:48 ` [RFC 02/65] target/riscv: correct the gvec IR called in gen_vec_rsub16_i64() frank.chang
2020-07-10 16:13 ` Richard Henderson
2020-07-10 10:48 ` [RFC 03/65] target/riscv: fix return value of do_opivx_widen() frank.chang
2020-07-10 16:14 ` Richard Henderson
2020-07-10 10:48 ` [RFC 04/65] target/riscv: fix vill bit index in vtype register frank.chang
2020-07-10 16:15 ` Richard Henderson
2020-07-10 10:48 ` [RFC 05/65] target/riscv: remove vsll.vi, vsrl.vi, vsra.vi insns from using gvec frank.chang
2020-07-10 16:27 ` Richard Henderson
2020-07-14 2:59 ` Frank Chang
2020-07-14 3:35 ` LIU Zhiwei
2020-07-14 4:39 ` Frank Chang
2020-07-14 13:21 ` Richard Henderson
2020-07-14 13:59 ` Frank Chang
2020-07-15 2:52 ` LIU Zhiwei
2020-07-10 10:48 ` [RFC 06/65] target/riscv: rvv-0.9: add vcsr register frank.chang
2020-07-10 17:02 ` Richard Henderson
2020-07-10 10:48 ` [RFC 07/65] target/riscv: rvv-0.9: add vector context status frank.chang
2020-07-10 17:26 ` Richard Henderson
2020-07-10 17:27 ` Richard Henderson
2020-07-10 10:48 ` [RFC 08/65] target/riscv: rvv-0.9: update mstatus_vs by tb_flags frank.chang
2020-07-10 17:28 ` Richard Henderson
2020-07-10 10:48 ` [RFC 09/65] target/riscv: rvv-0.9: add vlenb register frank.chang
2020-07-10 17:31 ` Richard Henderson
2020-07-10 10:48 ` [RFC 10/65] target/riscv: rvv-0.9: remove MLEN calculations frank.chang
2020-07-10 17:32 ` Richard Henderson
2020-07-10 10:48 ` [RFC 11/65] target/riscv: rvv-0.9: add fractional LMUL, VTA and VMA frank.chang
2020-07-10 17:45 ` Richard Henderson
2020-07-10 10:48 ` [RFC 12/65] target/riscv: rvv-0.9: update check functions frank.chang
2020-07-10 17:51 ` Richard Henderson
2020-07-13 2:10 ` Frank Chang
2020-07-10 10:48 ` [RFC 13/65] target/riscv: rvv-0.9: configure instructions frank.chang
2020-07-10 18:06 ` Richard Henderson
2020-07-13 2:07 ` Frank Chang
2020-07-10 10:48 ` [RFC 14/65] target/riscv: rvv-0.9: stride load and store instructions frank.chang
2020-07-10 18:15 ` Richard Henderson
2020-07-13 2:04 ` Frank Chang
2020-07-10 10:48 ` [RFC 15/65] target/riscv: rvv-0.9: index " frank.chang
2020-07-10 10:48 ` [RFC 16/65] target/riscv: rvv-0.9: fix address index overflow bug of indexed load/store insns frank.chang
2020-07-10 10:48 ` [RFC 17/65] target/riscv: rvv-0.9: fault-only-first unit stride load frank.chang
2020-07-10 10:48 ` [RFC 18/65] target/riscv: rvv-0.9: amo operations frank.chang
2020-07-10 10:48 ` [RFC 19/65] target/riscv: rvv-0.9: load/store whole register instructions frank.chang
2020-07-10 10:48 ` [RFC 20/65] target/riscv: rvv-0.9: update vext_max_elems() for load/store insns frank.chang
2020-07-10 10:48 ` [RFC 21/65] target/riscv: rvv-0.9: take fractional LMUL into vector max elements calculation frank.chang
2020-07-10 10:48 ` [RFC 22/65] target/riscv: rvv-0.9: floating-point square-root instruction frank.chang
2020-07-10 10:48 ` [RFC 23/65] target/riscv: rvv-0.9: floating-point classify instructions frank.chang
2020-07-10 10:48 ` [RFC 24/65] target/riscv: rvv-0.9: mask population count instruction frank.chang
2020-07-10 10:48 ` [RFC 25/65] target/riscv: rvv-0.9: find-first-set mask bit instruction frank.chang
2020-07-10 10:48 ` [RFC 26/65] target/riscv: rvv-0.9: set-X-first mask bit instructions frank.chang
2020-07-10 10:48 ` [RFC 27/65] target/riscv: rvv-0.9: iota instruction frank.chang
2020-07-10 10:48 ` [RFC 28/65] target/riscv: rvv-0.9: element index instruction frank.chang
2020-07-10 10:48 ` [RFC 29/65] target/riscv: rvv-0.9: integer scalar move instructions frank.chang
2020-07-10 10:48 ` [RFC 30/65] target/riscv: rvv-0.9: floating-point " frank.chang
2020-07-10 10:48 ` [RFC 31/65] target/riscv: rvv-0.9: whole register " frank.chang
2020-07-10 10:48 ` [RFC 32/65] target/riscv: rvv-0.9: integer extension instructions frank.chang
2020-07-10 10:48 ` [RFC 33/65] target/riscv: rvv-0.9: single-width averaging add and subtract instructions frank.chang
2020-07-10 10:48 ` [RFC 34/65] target/riscv: rvv-0.9: integer add-with-carry/subtract-with-borrow frank.chang
2020-07-10 10:48 ` [RFC 35/65] target/riscv: rvv-0.9: narrowing integer right shift instructions frank.chang
2020-07-10 10:48 ` [RFC 36/65] target/riscv: rvv-0.9: widening integer multiply-add instructions frank.chang
2020-07-10 10:48 ` [RFC 37/65] target/riscv: rvv-0.9: quad-widening " frank.chang
2020-07-10 10:48 ` [RFC 38/65] target/riscv: rvv-0.9: integer merge and move instructions frank.chang
2020-07-10 10:48 ` [RFC 39/65] target/riscv: rvv-0.9: single-width saturating add and subtract instructions frank.chang
2020-07-10 10:48 ` [RFC 40/65] target/riscv: rvv-0.9: integer comparison instructions frank.chang
2020-07-10 10:48 ` [RFC 41/65] target/riscv: rvv-0.9: floating-point compare instructions frank.chang
2020-07-10 10:48 ` [RFC 42/65] target/riscv: rvv-0.9: single-width integer reduction instructions frank.chang
2020-07-10 10:48 ` [RFC 43/65] target/riscv: rvv-0.9: widening " frank.chang
2020-07-10 10:48 ` [RFC 44/65] target/riscv: rvv-0.9: mask-register logical instructions frank.chang
2020-07-10 10:48 ` [RFC 45/65] target/riscv: rvv-0.9: register gather instructions frank.chang
2020-07-10 10:49 ` [RFC 46/65] target/riscv: rvv-0.9: slide instructions frank.chang
2020-07-10 10:49 ` [RFC 47/65] target/riscv: rvv-0.9: floating-point " frank.chang
2020-07-10 10:49 ` [RFC 48/65] target/riscv: rvv-0.9: narrowing fixed-point clip instructions frank.chang
2020-07-10 10:49 ` [RFC 49/65] target/riscv: rvv-0.9: floating-point move instructions frank.chang
2020-07-10 10:49 ` [RFC 50/65] target/riscv: rvv-0.9: floating-point/integer type-convert instructions frank.chang
2020-07-10 10:49 ` [RFC 51/65] target/riscv: rvv-0.9: single-width floating-point reduction frank.chang
2020-07-10 10:49 ` [RFC 52/65] target/riscv: rvv-0.9: widening floating-point reduction instructions frank.chang
2020-07-10 10:49 ` [RFC 53/65] target/riscv: rvv-0.9: single-width scaling shift instructions frank.chang
2020-07-10 10:49 ` [RFC 54/65] target/riscv: rvv-0.9: remove widening saturating scaled multiply-add frank.chang
2020-07-10 10:49 ` [RFC 55/65] target/riscv: rvv-0.9: remove vmford.vv and vmford.vf frank.chang
2020-07-10 10:49 ` [RFC 56/65] target/riscv: rvv-0.9: remove integer extract instruction frank.chang
2020-07-10 10:49 ` [RFC 57/65] target/riscv: rvv-0.9: floating-point min/max instructions frank.chang
2020-07-10 18:19 ` Alex Bennée
2020-07-10 10:49 ` [RFC 58/65] target/riscv: rvv-0.9: widening floating-point/integer type-convert frank.chang
2020-07-10 10:49 ` [RFC 59/65] target/riscv: rvv-0.9: narrowing " frank.chang
2020-07-10 10:49 ` [RFC 60/65] softfloat: add fp16 and uint8/int8 interconvert functions frank.chang
2020-07-10 12:07 ` Alex Bennée
2020-07-10 12:15 ` Frank Chang
2020-07-10 12:46 ` Alex Bennée
2020-07-10 13:13 ` Frank Chang
2020-07-10 14:59 ` Alex Bennée
2020-07-10 10:49 ` [RFC 61/65] fpu: fix float16 nan check frank.chang
2020-07-10 10:49 ` [RFC 62/65] fpu: add api to handle alternative sNaN propagation frank.chang
2020-07-10 12:15 ` Alex Bennée
2020-07-13 17:38 ` Chih-Min Chao [this message]
2020-07-10 10:49 ` [RFC 63/65] fpu: implement full set compare for fp16 frank.chang
2020-07-10 12:24 ` Alex Bennée
2020-07-10 12:26 ` Alex Bennée
2020-07-14 9:29 ` Chih-Min Chao
2020-07-10 10:49 ` [RFC 64/65] target/riscv: use softfloat lib float16 comparison functions frank.chang
2020-07-10 10:49 ` [RFC 65/65] target/riscv: bump to RVV 0.9 frank.chang
2020-07-10 21:43 ` [RFC 00/65] target/riscv: support vector extension v0.9 Alistair Francis
2020-07-13 2:02 ` Frank Chang
2020-07-13 15:57 ` Alistair Francis
2020-07-13 16:41 ` Richard Henderson
2020-07-13 16:44 ` Frank Chang
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=CAEiOBXXY33gxpBBmu0N8vauqFNXdnbWJiA58nUtHvRBtYFe-EQ@mail.gmail.com \
--to=chihmin.chao@sifive.com \
--cc=alex.bennee@linaro.org \
--cc=aurelien@aurel32.net \
--cc=frank.chang@sifive.com \
--cc=peter.maydell@linaro.org \
--cc=qemu-devel@nongnu.org \
--cc=qemu-riscv@nongnu.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).