On Mon, Jun 18, 2018 at 11:50:24AM -0400, John Arbuckle wrote: > Fix the helper_fpscr_clrbit() function so it correctly > sets the FEX and VX bits. > > Determining the value for the Floating Point Status and Control > Register's (FPSCR) FEX bit is suppose to be done like this: > > FEX = (VX & VE) | (OX & OE) | (UX & UE) | (ZX & ZE) | (XX & XE)) > > It is described as "the logical OR of all the floating-point exception bits > masked by their respective enable bits". It was not implemented correctly. > The value of FEX would stay on even when all other bits were set to off. > > The VX bit is described as "the logical OR of all of the invalid operation > exceptions". This bit was also not implemented correctly. It too would stay > on when all the other bits were set to off. > > My main source of information is an IBM document called: > > PowerPC Microprocessor Family: > The Programming Environments for 32-Bit Microprocessors > > Page 62 is where the FPSCR information is located. > > This is an older copy than the one I use but it is still very useful: > https://www.pdfdrive.net/powerpc-microprocessor-family-the-programming-environments-for-32-e3087633.html > > I use a G3 and G5 iMac to compare bit values with QEMU. This patch fixed all > the problems I was having with these bits. > > Signed-off-by: John Arbuckle <[email protected]> > --- > v2 changes: > - Removed the FPSCR_VX case because it is not a bit that can be set directly. > - Replaced previous code with predefined macros fpscr_ix and > fpscr_eex.
Thanks for the updated version, this is much easier to review.
This is definitely better than what we have, and I've applied it to
ppc-for-3.0. The existing code is pretty eye-watering, and longer
term I do wonder if it would be better to just compute the VX and FEX
bits when we actually read the fpscr, rather than incrementally.
I think there's also one case you've missed..
>
> target/ppc/fpu_helper.c | 28 ++++++++++++++++++++++++++++
> 1 file changed, 28 insertions(+)
>
> diff --git a/target/ppc/fpu_helper.c b/target/ppc/fpu_helper.c
> index d31a933cbb..7714bfe0f9 100644
> --- a/target/ppc/fpu_helper.c
> +++ b/target/ppc/fpu_helper.c
> @@ -325,6 +325,34 @@ void helper_fpscr_clrbit(CPUPPCState *env, uint32_t bit)
> case FPSCR_RN:
> fpscr_set_rounding_mode(env);
> break;
> + case FPSCR_VXSNAN:
> + case FPSCR_VXISI:
> + case FPSCR_VXIDI:
> + case FPSCR_VXZDZ:
> + case FPSCR_VXIMZ:
> + case FPSCR_VXVC:
> + case FPSCR_VXSOFT:
> + case FPSCR_VXSQRT:
> + case FPSCR_VXCVI:
> + if (!fpscr_ix) {
> + /* Set VX bit to zero */
> + env->fpscr &= ~(1 << FPSCR_VX);
> + }
This can clear the VX bit, which could affect the expected value of
the FEX bit, but you won't actually recompute it in that case.
> + break;
> + case FPSCR_OX:
> + case FPSCR_UX:
> + case FPSCR_ZX:
> + case FPSCR_XX:
> + case FPSCR_VE:
> + case FPSCR_OE:
> + case FPSCR_UE:
> + case FPSCR_ZE:
> + case FPSCR_XE:
> + if (!fpscr_eex) {
> + /* Set the FEX bit */
> + env->fpscr &= ~(1 << FPSCR_FEX);
> + }
> + break;
> default:
> break;
> }
--
David Gibson | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
signature.asc
Description: PGP signature
