Re: [Qemu-devel] [RFC PATCH v1] softfloat: Add round-to-odd rounding mode

[Bharata B Rao]

On Fri, Jan 20, 2017 at 10:28:22AM +0000, Peter Maydell wrote:
> On 20 January 2017 at 09:17, Bharata B Rao <address@hidden> 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 <address@hidden>
> > ---
> > 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.