Re: [Qemu-devel] [PATCH] target-i386: implement FPREM and FPREM1 using softfloat only

[Andreas Färber]

Am 28.06.2012 01:00, schrieb Catalin Patulea:
> Hey guys,
> 
> I've been hacking up the FPREM and FPREM1 i386 instructions (without KVM) to 
> be implemented using SoftFloat only. This was motivated by some correctness 
> issues
> that we noticed with the current implementation which follows the AMD 
> datasheet.I believe the datasheet explains the operation as if intermediate 
> results had "infinite" precision, but in this case intermediate rounding 
> causes a loss of precision at the final output.
> 
> My reference was TestFloat [1], specifically the floatx80_rem suite, which 
> tests the FPREM instruction quite thoroughly (FPREM1 is technically untested 
> but very similar). The current code fails about half the suite; with the 
> patch, all tests pass.
> 
> There are still lots of dark corners - the code originates from Bochs' copy 
> of SoftFloat and I tried to port the Bochs exception handling logic as much 
> as I could. Many details still elude me though (see comments in the code). 
> TestFloat does test some of the exception logic but not as thoroughly as I 
> would have liked. If anyone can offer some guidance here, I am happy to fix 
> up the patch.
> 
> That's about it.. let me know what you think.
> 
> Catalin
> 
> [1] http://www.jhauser.us/arithmetic/TestFloat.html
> 
> ---
>  fpu/softfloat.c         |  182 
> +++++++++++++++++++++++++++++++++++++++++++++++
>  fpu/softfloat.h         |    3 +
>  target-i386/op_helper.c |  163 +++++++++++++++++-------------------------
>  3 files changed, 249 insertions(+), 99 deletions(-)
> 
> diff --git a/fpu/softfloat.c b/fpu/softfloat.c
> index b29256a..9c6e4a3 100644
> --- a/fpu/softfloat.c
> +++ b/fpu/softfloat.c
> @@ -5234,6 +5234,16 @@ int floatx80_unordered_quiet( floatx80 a, floatx80 b 
> STATUS_PARAM )
>  }
>  
>  
> /*----------------------------------------------------------------------------
> +| Returns 1 if the extended double-precision floating-point value `a' is an
> +| unsupported; otherwise returns 0.
> +*----------------------------------------------------------------------------*/
> +int floatx80_is_unsupported(floatx80 a)
> +{
> +    return (extractFloatx80Exp(a) &&
> +            !(extractFloatx80Frac(a) & LIT64(0x8000000000000000)));
> +}

Disclaimer: It's been a while that I had to learn IEEE 754.

Unsupported for some i386 instruction? Or unsupported in the SoftFloat
library due to some limitation?

Please run scripts/checkpatch.pl for CODING_STYLE issues, I spotted one
slightly misplaced if brace, and one empty line at the bottom seemed to
have indentation (git-am complains about that, too).

The patch description will need to be cleaned up (not be letter-style).

I'm not too thrilled to introduce more uses of int32 (we started
converting int16 to int_fast16_t) but I won't veto. It would be nice if
you could review for any potential int32 vs. int32_t breakages though,
to not make things worse than they are.

Regards,
Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg