| Message ID | 1484903866-17940-1-git-send-email-bharata@linux.vnet.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 20 January 2017 at 09:17, Bharata B Rao <bharata@linux.vnet.ibm.com> wrote: > Power ISA 3.0 introduces a few quadruple precision floating point > instructions that support round-to-odd rounding mode. The > round-to-odd mode is explained as under: > > Let Z be the intermediate arithmetic result or the operand of a convert > operation. If Z can be represented exactly in the target format, the > result is Z. Otherwise the result is either Z1 or Z2 whichever is odd. > Here Z1 and Z2 are the next larger and smaller numbers representable > in the target format respectively. > > Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com> > --- > Changes in v1: > - Addressed rounding to infinity in roundAndPackFloat128(). > - Added 64bit round to odd implementation(roundAndPackFloat64()) as > it is needed by xscvqpdp instruction of Power ISA 3.0. > > v0: http://patchwork.ozlabs.org/patch/716956/ > > fpu/softfloat.c | 10 ++++++++++ > include/fpu/softfloat.h | 2 ++ > 2 files changed, 12 insertions(+) > > diff --git a/fpu/softfloat.c b/fpu/softfloat.c > index c295f31..b9b36ad 100644 > --- a/fpu/softfloat.c > +++ b/fpu/softfloat.c > @@ -623,6 +623,9 @@ static float64 roundAndPackFloat64(flag zSign, int zExp, uint64_t zSig, > case float_round_down: > roundIncrement = zSign ? 0x3ff : 0; > break; > + case float_round_to_odd: > + roundIncrement = (zSig & 0x200) ? 0 : 0x3ff; Are you sure that should be 0x200 and not 0x400 ? > + break; > default: > abort(); > } This isn't sufficient, because it won't do the right thing in the code which is picking between "round to infinity" and "round to largest/smallest representable number". That's phrased differently from the Float128 code but it's still there: return packFloat64( zSign, 0x7FF, - ( roundIncrement == 0 )); will generate a result with an all-zeros mantissa for roundIncrement == 0 (ie go to infinity) and an all-ones mantissa otherwise (ie go to largest-representable). That works for the existing cases but it doesn't work for round_to_odd. How are you testing these patches? You ought to be able to compare the results against a known-good implementation for a bunch of these corner cases and a lot of random input values, which is the best way to be confident there aren't bugs in them. (risu should be able to do this since it supports PPC now, though it can miss things depending on the number of iterations you ask it to do.) > @@ -1149,6 +1152,9 @@ static float128 roundAndPackFloat128(flag zSign, int32_t zExp, > case float_round_down: > increment = zSign && zSig2; > break; > + case float_round_to_odd: > + increment = !(zSig1 & 0x1) && zSig2; > + break; > default: > abort(); > } > @@ -1168,6 +1174,7 @@ static float128 roundAndPackFloat128(flag zSign, int32_t zExp, > if ( ( roundingMode == float_round_to_zero ) > || ( zSign && ( roundingMode == float_round_up ) ) > || ( ! zSign && ( roundingMode == float_round_down ) ) > + || ( roundingMode == float_round_to_odd ) > ) { > return > packFloat128( > @@ -1215,6 +1222,9 @@ static float128 roundAndPackFloat128(flag zSign, int32_t zExp, > case float_round_down: > increment = zSign && zSig2; > break; > + case float_round_to_odd: > + increment = !(zSig1 & 0x1) && zSig2; > + break; > default: > abort(); > } > diff --git a/include/fpu/softfloat.h b/include/fpu/softfloat.h > index 842ec6b..8a39028 100644 > --- a/include/fpu/softfloat.h > +++ b/include/fpu/softfloat.h > @@ -180,6 +180,8 @@ enum { > float_round_up = 2, > float_round_to_zero = 3, > float_round_ties_away = 4, > + /* Not an IEEE rounding mode: round to the closest odd mantissa value */ > + float_round_to_odd = 5, > }; > > /*---------------------------------------------------------------------------- thanks -- PMM
On Fri, Jan 20, 2017 at 10:28:22AM +0000, Peter Maydell wrote: > On 20 January 2017 at 09:17, Bharata B Rao <bharata@linux.vnet.ibm.com> wrote: > > Power ISA 3.0 introduces a few quadruple precision floating point > > instructions that support round-to-odd rounding mode. The > > round-to-odd mode is explained as under: > > > > Let Z be the intermediate arithmetic result or the operand of a convert > > operation. If Z can be represented exactly in the target format, the > > result is Z. Otherwise the result is either Z1 or Z2 whichever is odd. > > Here Z1 and Z2 are the next larger and smaller numbers representable > > in the target format respectively. > > > > Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com> > > --- > > Changes in v1: > > - Addressed rounding to infinity in roundAndPackFloat128(). > > - Added 64bit round to odd implementation(roundAndPackFloat64()) as > > it is needed by xscvqpdp instruction of Power ISA 3.0. > > > > v0: http://patchwork.ozlabs.org/patch/716956/ > > > > fpu/softfloat.c | 10 ++++++++++ > > include/fpu/softfloat.h | 2 ++ > > 2 files changed, 12 insertions(+) > > > > diff --git a/fpu/softfloat.c b/fpu/softfloat.c > > index c295f31..b9b36ad 100644 > > --- a/fpu/softfloat.c > > +++ b/fpu/softfloat.c > > @@ -623,6 +623,9 @@ static float64 roundAndPackFloat64(flag zSign, int zExp, uint64_t zSig, > > case float_round_down: > > roundIncrement = zSign ? 0x3ff : 0; > > break; > > + case float_round_to_odd: > > + roundIncrement = (zSig & 0x200) ? 0 : 0x3ff; > > Are you sure that should be 0x200 and not 0x400 ? You are right, it should have been 0x400 as we are discarding lower 10 precision bits. The even or odd check should have been for the next higher significant bit to that of these 10 discarded bits. > > > + break; > > default: > > abort(); > > } > > This isn't sufficient, because it won't do the right thing > in the code which is picking between "round to infinity" and > "round to largest/smallest representable number". That's > phrased differently from the Float128 code but it's still > there: > > return packFloat64( zSign, 0x7FF, - ( roundIncrement == 0 )); > > will generate a result with an all-zeros mantissa for > roundIncrement == 0 (ie go to infinity) and an all-ones > mantissa otherwise (ie go to largest-representable). > That works for the existing cases but it doesn't work > for round_to_odd. Based on my understanding of your and Richard's clarification, we shouldn't overflow to infinity in round-to-odd mode. Like I did for 128 bit case where we return the max possible value in the similar situation, I suppose we should explicitly take care of returning max 64bit value here for round-to-odd case ? > > How are you testing these patches? You ought to be able > to compare the results against a known-good implementation > for a bunch of these corner cases and a lot of random > input values, which is the best way to be confident there > aren't bugs in them. (risu should be able to do this since > it supports PPC now, though it can miss things depending > on the number of iterations you ask it to do.) I have been manually checking the result of new PPC instructions like xsdivqp[o], xsmulqp[o], xscvqpdp[0]. But that is clearly not enough. Let me look at RISU and do a more thorough corner cases testing before posting the next version. Thanks for your review. Regards, Bharata.
On 20 January 2017 at 11:36, Bharata B Rao <bharata@linux.vnet.ibm.com> wrote: > On Fri, Jan 20, 2017 at 10:28:22AM +0000, Peter Maydell wrote: >> This isn't sufficient, because it won't do the right thing >> in the code which is picking between "round to infinity" and >> "round to largest/smallest representable number". That's >> phrased differently from the Float128 code but it's still >> there: >> >> return packFloat64( zSign, 0x7FF, - ( roundIncrement == 0 )); >> >> will generate a result with an all-zeros mantissa for >> roundIncrement == 0 (ie go to infinity) and an all-ones >> mantissa otherwise (ie go to largest-representable). >> That works for the existing cases but it doesn't work >> for round_to_odd. > > Based on my understanding of your and Richard's clarification, we > shouldn't overflow to infinity in round-to-odd mode. Like I did for 128 bit > case where we return the max possible value in the similar situation, I > suppose we should explicitly take care of returning max 64bit value here > for round-to-odd case ? I would suggest something like bool overflow_to_inf = roundingMode != float_round_to_odd && roundIncrement != 0; float_raise(float_flag_overflow | float_flag_inexact, status); return packFloat64(zSign, 0x7FF, -(!overflow_to_inf)); as the contents of the if() {} body for overflow detected. thanks -- PMM
Hi,
Your series seems to have some coding style problems. See output below for
more information:
Subject: [Qemu-devel] [RFC PATCH v1] softfloat: Add round-to-odd rounding mode
Message-id: 1484903866-17940-1-git-send-email-bharata@linux.vnet.ibm.com
Type: series
=== TEST SCRIPT BEGIN ===
#!/bin/bash
BASE=base
n=1
total=$(git log --oneline $BASE.. | wc -l)
failed=0
# Useful git options
git config --local diff.renamelimit 0
git config --local diff.renames True
commits="$(git log --format=%H --reverse $BASE..)"
for c in $commits; do
echo "Checking PATCH $n/$total: $(git log -n 1 --format=%s $c)..."
if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -; then
failed=1
echo
fi
n=$((n+1))
done
exit $failed
=== TEST SCRIPT END ===
Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
Switched to a new branch 'test'
0d1f7ea softfloat: Add round-to-odd rounding mode
=== OUTPUT BEGIN ===
Checking PATCH 1/1: softfloat: Add round-to-odd rounding mode...
ERROR: space prohibited after that open parenthesis '('
#47: FILE: fpu/softfloat.c:1177:
+ || ( roundingMode == float_round_to_odd )
ERROR: space prohibited before that close parenthesis ')'
#47: FILE: fpu/softfloat.c:1177:
+ || ( roundingMode == float_round_to_odd )
total: 2 errors, 0 warnings, 42 lines checked
Your patch has style problems, please review. If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
=== OUTPUT END ===
Test command exited with code: 1
---
Email generated automatically by Patchew [http://patchew.org/].
Please send your feedback to patchew-devel@freelists.org
diff --git a/fpu/softfloat.c b/fpu/softfloat.c index c295f31..b9b36ad 100644 --- a/fpu/softfloat.c +++ b/fpu/softfloat.c @@ -623,6 +623,9 @@ static float64 roundAndPackFloat64(flag zSign, int zExp, uint64_t zSig, case float_round_down: roundIncrement = zSign ? 0x3ff : 0; break; + case float_round_to_odd: + roundIncrement = (zSig & 0x200) ? 0 : 0x3ff; + break; default: abort(); } @@ -1149,6 +1152,9 @@ static float128 roundAndPackFloat128(flag zSign, int32_t zExp, case float_round_down: increment = zSign && zSig2; break; + case float_round_to_odd: + increment = !(zSig1 & 0x1) && zSig2; + break; default: abort(); } @@ -1168,6 +1174,7 @@ static float128 roundAndPackFloat128(flag zSign, int32_t zExp, if ( ( roundingMode == float_round_to_zero ) || ( zSign && ( roundingMode == float_round_up ) ) || ( ! zSign && ( roundingMode == float_round_down ) ) + || ( roundingMode == float_round_to_odd ) ) { return packFloat128( @@ -1215,6 +1222,9 @@ static float128 roundAndPackFloat128(flag zSign, int32_t zExp, case float_round_down: increment = zSign && zSig2; break; + case float_round_to_odd: + increment = !(zSig1 & 0x1) && zSig2; + break; default: abort(); } diff --git a/include/fpu/softfloat.h b/include/fpu/softfloat.h index 842ec6b..8a39028 100644 --- a/include/fpu/softfloat.h +++ b/include/fpu/softfloat.h @@ -180,6 +180,8 @@ enum { float_round_up = 2, float_round_to_zero = 3, float_round_ties_away = 4, + /* Not an IEEE rounding mode: round to the closest odd mantissa value */ + float_round_to_odd = 5, }; /*----------------------------------------------------------------------------
Power ISA 3.0 introduces a few quadruple precision floating point instructions that support round-to-odd rounding mode. The round-to-odd mode is explained as under: Let Z be the intermediate arithmetic result or the operand of a convert operation. If Z can be represented exactly in the target format, the result is Z. Otherwise the result is either Z1 or Z2 whichever is odd. Here Z1 and Z2 are the next larger and smaller numbers representable in the target format respectively. Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com> --- Changes in v1: - Addressed rounding to infinity in roundAndPackFloat128(). - Added 64bit round to odd implementation(roundAndPackFloat64()) as it is needed by xscvqpdp instruction of Power ISA 3.0. v0: http://patchwork.ozlabs.org/patch/716956/ fpu/softfloat.c | 10 ++++++++++ include/fpu/softfloat.h | 2 ++ 2 files changed, 12 insertions(+)