| Message ID | 20180917211858.21795-1-programmingkidx@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | fix setting the FPSCR[FR] bit | expand |
On 17 September 2018 at 22:18, John Arbuckle <programmingkidx@gmail.com> wrote: > https://www.nxp.com/files-static/product/doc/MPCFPE32B.pdf > Page 2-8 in table 2-4 is where the description of this bit can be found. > > It is described as: > > Floating-point fraction rounded. The last arithmetic, rounding, or conversion instruction incremented the fraction. This bit is NOT sticky. > > This patch actually implements the setting and unsetting of this bit. > > Signed-off-by: John Arbuckle <programmingkidx@gmail.com> > --- > fpu/softfloat.c | 12 ++++++++++-- > include/fpu/softfloat-types.h | 1 + > target/ppc/fpu_helper.c | 12 ++++++++++++ > 3 files changed, 23 insertions(+), 2 deletions(-) > > diff --git a/fpu/softfloat.c b/fpu/softfloat.c > index 59ca356d0e..c5378ae9e8 100644 > --- a/fpu/softfloat.c > +++ b/fpu/softfloat.c > @@ -751,9 +751,17 @@ float64 __attribute__((flatten)) float64_add(float64 a, float64 b, > { > FloatParts pa = float64_unpack_canonical(a, status); > FloatParts pb = float64_unpack_canonical(b, status); > - FloatParts pr = addsub_floats(pa, pb, false, status); > + FloatParts intermediate_parts = addsub_floats(pa, pb, false, status); > > - return float64_round_pack_canonical(pr, status); > + float64 rounded_result = float64_round_pack_canonical(intermediate_parts, > + status); > + FloatParts rounded_parts = float64_unpack_canonical(rounded_result, status); > + > + if (rounded_parts.frac != intermediate_parts.frac) { > + float_raise(float_flag_round, status); > + } > + > + return rounded_result; > } Only changing the add instruction looks very definitely wrong. Doing a pack-unpack-compare looks dubious. I think that you can do this by using the existing overflow and inexact flags. If that is not possible (but I'm pretty sure it should be doable), then the next best approach is probably to have the new float flag be set at the right point in the round-and-pack code. thanks -- PMM
> On Sep 17, 2018, at 5:25 PM, Peter Maydell <peter.maydell@linaro.org> wrote: > > On 17 September 2018 at 22:18, John Arbuckle <programmingkidx@gmail.com> wrote: >> https://www.nxp.com/files-static/product/doc/MPCFPE32B.pdf >> Page 2-8 in table 2-4 is where the description of this bit can be found. >> >> It is described as: >> >> Floating-point fraction rounded. The last arithmetic, rounding, or conversion instruction incremented the fraction. This bit is NOT sticky. >> >> This patch actually implements the setting and unsetting of this bit. >> >> Signed-off-by: John Arbuckle <programmingkidx@gmail.com> >> --- >> fpu/softfloat.c | 12 ++++++++++-- >> include/fpu/softfloat-types.h | 1 + >> target/ppc/fpu_helper.c | 12 ++++++++++++ >> 3 files changed, 23 insertions(+), 2 deletions(-) >> >> diff --git a/fpu/softfloat.c b/fpu/softfloat.c >> index 59ca356d0e..c5378ae9e8 100644 >> --- a/fpu/softfloat.c >> +++ b/fpu/softfloat.c >> @@ -751,9 +751,17 @@ float64 __attribute__((flatten)) float64_add(float64 a, float64 b, >> { >> FloatParts pa = float64_unpack_canonical(a, status); >> FloatParts pb = float64_unpack_canonical(b, status); >> - FloatParts pr = addsub_floats(pa, pb, false, status); >> + FloatParts intermediate_parts = addsub_floats(pa, pb, false, status); >> >> - return float64_round_pack_canonical(pr, status); >> + float64 rounded_result = float64_round_pack_canonical(intermediate_parts, >> + status); >> + FloatParts rounded_parts = float64_unpack_canonical(rounded_result, status); >> + >> + if (rounded_parts.frac != intermediate_parts.frac) { >> + float_raise(float_flag_round, status); >> + } >> + >> + return rounded_result; >> } > > Only changing the add instruction looks very definitely wrong. > Doing a pack-unpack-compare looks dubious. Now that I think about it I see it could be done differently (more efficiently). > I think that you can do this by using the existing overflow and > inexact flags. I'm not sure how I would do this. Do you think they are mutually exclusive? > If that is not possible (but I'm pretty sure it should > be doable), then the next best approach is probably to have the new > float flag be set at the right point in the round-and-pack code. This sounds like a good idea. The round_canonical() function is where I would place such code. This function is used by several arithmetic instructions so it might be able to more than just the add function. Thank you for reviewing my patch.
On 18 September 2018 at 00:18, Programmingkid <programmingkidx@gmail.com> wrote: > >> On Sep 17, 2018, at 5:25 PM, Peter Maydell <peter.maydell@linaro.org> wrote: >> >> On 17 September 2018 at 22:18, John Arbuckle <programmingkidx@gmail.com> wrote: >>> https://www.nxp.com/files-static/product/doc/MPCFPE32B.pdf >>> Page 2-8 in table 2-4 is where the description of this bit can be found. >>> >>> It is described as: >>> >>> Floating-point fraction rounded. The last arithmetic, rounding, or conversion instruction incremented the fraction. This bit is NOT sticky. >>> >>> This patch actually implements the setting and unsetting of this bit. >>> >>> Signed-off-by: John Arbuckle <programmingkidx@gmail.com> >>> --- >>> fpu/softfloat.c | 12 ++++++++++-- >>> include/fpu/softfloat-types.h | 1 + >>> target/ppc/fpu_helper.c | 12 ++++++++++++ >>> 3 files changed, 23 insertions(+), 2 deletions(-) >>> >>> diff --git a/fpu/softfloat.c b/fpu/softfloat.c >>> index 59ca356d0e..c5378ae9e8 100644 >>> --- a/fpu/softfloat.c >>> +++ b/fpu/softfloat.c >>> @@ -751,9 +751,17 @@ float64 __attribute__((flatten)) float64_add(float64 a, float64 b, >>> { >>> FloatParts pa = float64_unpack_canonical(a, status); >>> FloatParts pb = float64_unpack_canonical(b, status); >>> - FloatParts pr = addsub_floats(pa, pb, false, status); >>> + FloatParts intermediate_parts = addsub_floats(pa, pb, false, status); >>> >>> - return float64_round_pack_canonical(pr, status); >>> + float64 rounded_result = float64_round_pack_canonical(intermediate_parts, >>> + status); >>> + FloatParts rounded_parts = float64_unpack_canonical(rounded_result, status); >>> + >>> + if (rounded_parts.frac != intermediate_parts.frac) { >>> + float_raise(float_flag_round, status); >>> + } >>> + >>> + return rounded_result; >>> } >> >> Only changing the add instruction looks very definitely wrong. >> Doing a pack-unpack-compare looks dubious. > > Now that I think about it I see it could be done differently (more efficiently). > >> I think that you can do this by using the existing overflow and >> inexact flags. > > I'm not sure how I would do this. Do you think they are mutually exclusive? See my previous email -- the spec suggests that "round" is "inexact but not overflow". If so you should be able to implement it purely in the target/ppc code. thanks -- PMM
> On Sep 17, 2018, at 7:46 PM, Peter Maydell <peter.maydell@linaro.org> wrote: > > On 18 September 2018 at 00:18, Programmingkid <programmingkidx@gmail.com> wrote: >> >>> On Sep 17, 2018, at 5:25 PM, Peter Maydell <peter.maydell@linaro.org> wrote: >>> >>> On 17 September 2018 at 22:18, John Arbuckle <programmingkidx@gmail.com> wrote: >>>> https://www.nxp.com/files-static/product/doc/MPCFPE32B.pdf >>>> Page 2-8 in table 2-4 is where the description of this bit can be found. >>>> >>>> It is described as: >>>> >>>> Floating-point fraction rounded. The last arithmetic, rounding, or conversion instruction incremented the fraction. This bit is NOT sticky. >>>> >>>> This patch actually implements the setting and unsetting of this bit. >>>> >>>> Signed-off-by: John Arbuckle <programmingkidx@gmail.com> >>>> --- >>>> fpu/softfloat.c | 12 ++++++++++-- >>>> include/fpu/softfloat-types.h | 1 + >>>> target/ppc/fpu_helper.c | 12 ++++++++++++ >>>> 3 files changed, 23 insertions(+), 2 deletions(-) >>>> >>>> diff --git a/fpu/softfloat.c b/fpu/softfloat.c >>>> index 59ca356d0e..c5378ae9e8 100644 >>>> --- a/fpu/softfloat.c >>>> +++ b/fpu/softfloat.c >>>> @@ -751,9 +751,17 @@ float64 __attribute__((flatten)) float64_add(float64 a, float64 b, >>>> { >>>> FloatParts pa = float64_unpack_canonical(a, status); >>>> FloatParts pb = float64_unpack_canonical(b, status); >>>> - FloatParts pr = addsub_floats(pa, pb, false, status); >>>> + FloatParts intermediate_parts = addsub_floats(pa, pb, false, status); >>>> >>>> - return float64_round_pack_canonical(pr, status); >>>> + float64 rounded_result = float64_round_pack_canonical(intermediate_parts, >>>> + status); >>>> + FloatParts rounded_parts = float64_unpack_canonical(rounded_result, status); >>>> + >>>> + if (rounded_parts.frac != intermediate_parts.frac) { >>>> + float_raise(float_flag_round, status); >>>> + } >>>> + >>>> + return rounded_result; >>>> } >>> >>> Only changing the add instruction looks very definitely wrong. >>> Doing a pack-unpack-compare looks dubious. >> >> Now that I think about it I see it could be done differently (more efficiently). >> >>> I think that you can do this by using the existing overflow and >>> inexact flags. >> >> I'm not sure how I would do this. Do you think they are mutually exclusive? > > See my previous email -- the spec suggests that "round" is > "inexact but not overflow". I couldn't find anything in my pdf document about round being defined as inexact but not overflow. Could you send me a link to your document? > If so you should be able to implement > it purely in the target/ppc code. The Fraction Rounded (FR) flag is set when the intermediate result of an IEEE 754 calculation does not match the rounded answer. The only way to find out if the intermediate result is not equal to the rounded answer is to do a comparison in the code that does the actual calculations. For the floating point add instruction the code that handles the calculation is the fpu/softfloat.c:float64_add() function. One way to implement the Fraction Rounded flag in target/ppc only would be to implement the actual calculation code in the target/ppc/fpu_helper.c file. @David Gibson - did you want to do that?
On 18 September 2018 at 15:34, Programmingkid <programmingkidx@gmail.com> wrote: > On Sep 17, 2018, at 7:46 PM, Peter Maydell <peter.maydell@linaro.org> wrote: >> See my previous email -- the spec suggests that "round" is >> "inexact but not overflow". > > I couldn't find anything in my pdf document about round being defined as inexact but not overflow. Could you send me a link to your document? My suggestion was based on the doc at: https://openpowerfoundation.org/?resource_lib=power-isa-version-3-0 which also lets you download the 2.07B spec, which says on page 115: Floating-Point Fraction Inexact (FI) The last Arithmetic or Rounding and Conversion instruction either produced an inexact result during rounding or caused a disabled Overflow Exception. which I took to mean that Inexact is "rounded or overflow", so rounded should be "inexact but not overflow". However, reading section 4.6 suggests that it's not quite that simple, because FR is only set if the fraction part is *incremented*, whereas Inexact covers also cases where the result is inexact but got rounded down. So I now think that we need to implement this flag properly in softfloat, by having the code that does rounding and sets float_flag_inexact also set float_flag_round in the correct places. I think also that float_flag_round is a slightly confusing name for these semantics when it's in the softfloat code rather than the ppc specific code, because it isn't set only when the result is rounded, but when the fractional part is specifically rounded up. Maybe float_flag_frac_inc ? thanks -- PMM
diff --git a/fpu/softfloat.c b/fpu/softfloat.c index 59ca356d0e..c5378ae9e8 100644 --- a/fpu/softfloat.c +++ b/fpu/softfloat.c @@ -751,9 +751,17 @@ float64 __attribute__((flatten)) float64_add(float64 a, float64 b, { FloatParts pa = float64_unpack_canonical(a, status); FloatParts pb = float64_unpack_canonical(b, status); - FloatParts pr = addsub_floats(pa, pb, false, status); + FloatParts intermediate_parts = addsub_floats(pa, pb, false, status); - return float64_round_pack_canonical(pr, status); + float64 rounded_result = float64_round_pack_canonical(intermediate_parts, + status); + FloatParts rounded_parts = float64_unpack_canonical(rounded_result, status); + + if (rounded_parts.frac != intermediate_parts.frac) { + float_raise(float_flag_round, status); + } + + return rounded_result; } float16 __attribute__((flatten)) float16_sub(float16 a, float16 b, diff --git a/include/fpu/softfloat-types.h b/include/fpu/softfloat-types.h index 2aae6a89b1..1d124e659c 100644 --- a/include/fpu/softfloat-types.h +++ b/include/fpu/softfloat-types.h @@ -147,6 +147,7 @@ enum { enum { float_flag_invalid = 1, + float_flag_round = 2, float_flag_divbyzero = 4, float_flag_overflow = 8, float_flag_underflow = 16, diff --git a/target/ppc/fpu_helper.c b/target/ppc/fpu_helper.c index b9bb1b856e..eed4f1a650 100644 --- a/target/ppc/fpu_helper.c +++ b/target/ppc/fpu_helper.c @@ -581,6 +581,7 @@ static void do_float_check_status(CPUPPCState *env, uintptr_t raddr) CPUState *cs = CPU(ppc_env_get_cpu(env)); int status = get_float_exception_flags(&env->fp_status); bool inexact_happened = false; + bool round_happened = false; if (status & float_flag_overflow) { float_overflow_excp(env); @@ -591,11 +592,22 @@ static void do_float_check_status(CPUPPCState *env, uintptr_t raddr) inexact_happened = true; } + /* if the round flag was set */ + if (status & float_flag_round) { + round_happened = true; + env->fpscr |= 1 << FPSCR_FR; + } + /* if the inexact flag was not set */ if (inexact_happened == false) { env->fpscr &= ~(1 << FPSCR_FI); /* clear the FPSCR[FI] bit */ } + /* if the floating-point fraction rounded bit was not set */ + if (round_happened == false) { + env->fpscr &= ~(1 << FPSCR_FR); /* clear the FPSCR[FR] bit */ + } + if (cs->exception_index == POWERPC_EXCP_PROGRAM && (env->error_code & POWERPC_EXCP_FP)) { /* Differred floating-point exception after target FPR update */
https://www.nxp.com/files-static/product/doc/MPCFPE32B.pdf Page 2-8 in table 2-4 is where the description of this bit can be found. It is described as: Floating-point fraction rounded. The last arithmetic, rounding, or conversion instruction incremented the fraction. This bit is NOT sticky. This patch actually implements the setting and unsetting of this bit. Signed-off-by: John Arbuckle <programmingkidx@gmail.com> --- fpu/softfloat.c | 12 ++++++++++-- include/fpu/softfloat-types.h | 1 + target/ppc/fpu_helper.c | 12 ++++++++++++ 3 files changed, 23 insertions(+), 2 deletions(-)