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 <programmingk...@gmail.com> > --- > 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