[RFC] Fix FPSCR.FI flag not being handled correctly #76

vcoracolombo · 2022-03-09T12:35:43Z

This is an RFC on a rework on how FPSCR flags are handled in fpu_helper.c. The objective is to fix the FI bit not being handled correctly in some cases, specifically when the ISA does not list the bit as being modified by the instruction. This should cause the bit to be kept unchanged. However, as of now QEMU is clearing or setting FI for every instruction, even when it shouldn't.
A list of instructions that are misbehaving:

xscmp* e xvcmp*
xsmax* e xvmax*
xsmin* e xvmin*
xstdivdp e similares
xvadd*
xvmul*
xvdiv*
xvcv*
xvmadd* e xvmsub*
xvnmadd* e xvnmsub*
xvr*

The idea is to introduce a mask in each helper, containing the information on what FPSCR flags that instruction should alter. This way, instructions that does not alter FI can be made to keep it untouched.

For this first RFC I implemented a new function update_fpscr to replace do_float_check_status. The objective is to not only allow the masking of some flags, but also trying to make the code cleaner by better concentrating the flags and exceptions handling in this function. I then used update_fpscr in the multiply-add instructions (xv[n]madd* and xv[n]msub*) and convert instructions (xvcv*) as an example of its usage. If the idea is approved other instructions would then be refactored for the final version.

vcoracolombo · 2022-03-09T12:43:11Z

target/ppc/fpu_helper.c

@@ -383,6 +383,21 @@ static inline void float_inexact_excp(CPUPPCState *env)
    }
 }

+static inline void float_inexact_excp2(CPUPPCState *env)


This function is just a placeholder to avoid changing the original one and break other functions that were not addressed yet. The idea is to remove the old implementation when everything is converted to use update_fpscr

vcoracolombo · 2022-03-09T12:44:33Z

target/ppc/fpu_helper.c

@@ -2939,10 +2986,8 @@ VSX_CVT_FP_TO_INT_VECTOR(xscvqpuwz, float128, uint32, f128, VsrD(0), 0x0ULL)
 *   ttp   - target type (float32 or float64)
 *   sfld  - source vsr_t field
 *   tfld  - target vsr_t field
- *   jdef  - definition of the j index (i or 2*i)
- *   sfprf - set FPRF
 */


This RFC is still missing comments that should be added for a final version (here, it's missing the fpscr_mask documentation)

target/ppc/fpu_helper.c

mferst · 2022-03-09T17:17:19Z

target/ppc/fpu_helper.c

+    int flags = get_float_exception_flags(&env->fp_status);
+
+    if ((mask & FP_VXSNAN) && (flags & float_flag_invalid_snan)) {
+        float_invalid_op_vxsnan(env, GETPC());


GETPC() should only be called in the top-level helper. Also, this method will not return if it raises the exception.

Changed this in d1450ba. Could you take a look and see if the new approach is better?

It seems better. I guess we can open update_fpscr in finish_fp_operation since there isn't any other caller. The same should happen with raise_excp when we replace do_float_check_status completely.

Changed in 411bc74 and made raise_excp inline

I'd remove the inline from raise_excp, the compiler should be smart enough to decide whether it's better to inline, tail-call, etc.

mferst · 2022-03-11T19:26:19Z

target/ppc/fpu_helper.c

@@ -3351,8 +3319,6 @@ void helper_xssubqp(CPUPPCState *env, uint32_t opcode,
    ppc_vsr_t t = *xt;
    float_status tstat;

-    helper_reset_fpstatus(env);


There are more calls to helper_reset_fpstatus through gen_reset_fpstatus. We can remove them if the insns calls gen_helper_float_check_status (directly or with gen_compute_fprf_float64).

Signed-off-by: Víctor Colombo <[email protected]>

vcoracolombo · 2022-03-22T17:00:02Z

I decided to remove the work on getting rid of helper_reset_fpstatus from this RFC to make it more concise. I might open another PR for that change later

alqotel · 2022-03-22T18:46:20Z

target/ppc/fpu_helper.c

+    env->fpscr |= FP_VX;
+    /* Update the floating-point exception summary */
+    env->fpscr |= FP_FX;
+    if (fpscr_ve != 0) {


Richard Henderson recommended to avoid hidden uses of env (like in fpscr_ve), so use env->fpscr & FP_VE instead.

We should probably get rid of these fpscr_* and msr_* defines after the freeze...

alqotel · 2022-03-22T18:52:39Z

target/ppc/fpu_helper.c

+/* TODO: this is not a 'finish' anymore, find a better name */
+static void finish_invalid_op_arith2(CPUPPCState *env, int op, bool set_fpcc)
+{
+    env->fpscr &= ~(FP_FR | FP_FI);


From what I understand to set/unset these bits you pass them through the mask to finish_fp_operation, so I don't understand why this function unset them even if they're not a part of the mask.

The ISA says in Invalid Operation Exception section that

If the operation is an arithmetic, Floating Round to Single-Precision, Floating Round to Integer, or convert to integer operation,
...
FR FI are set to zero

I agree this is a weird side effect hidden in this function. I was thinking this would be necessary to accommodate some cases in the future like when invalid should no set inexact (as was happening here be2e904#r68851624 but seems to be fixed in a more recent version) but this might not be case. I did some more tests and I think I'll drop this line.

Thanks for your review

IIUC, these methods would only be called when the bits they change are in the mask. That means that the knowledge of which bits can be changed is in the caller, but knowledge of which bits will change is in the callee. I think it makes it harder to reason about the code. E.g., if we need to change something in this method, we'll need to check all callers (and the calls to the callers) to see if the change is allowed.

That's not far from the current situation, but it feels like we can do better. Maybe we shouldn't change env->fpscr directly, but return the bits to be changed, and let the caller do a env->fpscr = (old_fpscr & ~mask) | (new_fpscr & mask). The problem then becomes the side effects of the methods, like setting env->error_code.

Another option is to follow the FPCC approach and have a bool set_fifr argument, which probably supersedes the need for a mask argument but may not be generic enough to solve other incorrect-bit-is-setted problems.

alqotel · 2022-03-22T18:54:26Z

target/ppc/fpu_helper.c

+    env->fpscr |= FP_XX;
+    /* Update the floating-point exception summary */
+    env->fpscr |= FP_FX;
+    if (fpscr_xe != 0) {


Same as fpscr_ve, change it to env->fpscr & FP_XE

alqotel · 2022-03-24T17:10:19Z

target/ppc/fpu_helper.c

+/* TODO: this is not a 'finish' anymore, find a better name */
+static void finish_invalid_op_arith2(CPUPPCState *env, int op, bool set_fpcc)
+{
+    if (fpscr_ve == 0) {


I missed this one when pointing the fpscr_* to change, this one should be env->fpscr & FP_VE too

vcoracolombo · 2022-05-25T16:19:50Z

Dropped in favor of #81

vcoracolombo self-assigned this Mar 9, 2022

vcoracolombo commented Mar 9, 2022

View reviewed changes

vcoracolombo requested review from alqotel, lrcoutinho and mferst March 9, 2022 14:01

mferst reviewed Mar 9, 2022

View reviewed changes

vcoracolombo requested a review from mferst March 11, 2022 12:17

mferst reviewed Mar 11, 2022

View reviewed changes

vcoracolombo mentioned this pull request Mar 17, 2022

Accumulator instructions #77

Closed

vcoracolombo force-pushed the vccolombo-fpscr-fi branch 2 times, most recently from 3dce0dc to 3d70cd5 Compare March 22, 2022 16:56

vcoracolombo added 2 commits March 22, 2022 13:57

target/ppc: Fix FPSCR.FI flag for VSX_CVT_INT_TO_FP helper

3f40cb2

Signed-off-by: Víctor Colombo <[email protected]>

target/ppc: Fix FPSCR.FI flag for VSX_MADD helper

6dd0995

Signed-off-by: Víctor Colombo <[email protected]>

vcoracolombo force-pushed the vccolombo-fpscr-fi branch from 3d70cd5 to 6dd0995 Compare March 22, 2022 16:57

vcoracolombo requested a review from mferst March 22, 2022 17:00

alqotel reviewed Mar 22, 2022

View reviewed changes

vcoracolombo added 2 commits March 23, 2022 08:23

fixup! target/ppc: Fix FPSCR.FI flag for VSX_CVT_INT_TO_FP helper

b276447

fixup! target/ppc: Fix FPSCR.FI flag for VSX_MADD helper

8c6e2a6

alqotel reviewed Mar 24, 2022

View reviewed changes

vcoracolombo mentioned this pull request Mar 24, 2022

[RFC] PPC hardfpu for instructions that don't alter FPSCR.FI #80

Open

vcoracolombo added 2 commits March 29, 2022 12:34

fixup! target/ppc: Fix FPSCR.FI flag for VSX_CVT_INT_TO_FP helper

470df3f

fixup! fixup! target/ppc: Fix FPSCR.FI flag for VSX_MADD helper

70ca489

vcoracolombo mentioned this pull request Mar 31, 2022

[RFC] (Alternative) Fix FPSCR.FI and rework excp methods #81

Open

vcoracolombo closed this May 25, 2022

[RFC] Fix FPSCR.FI flag not being handled correctly #76

[RFC] Fix FPSCR.FI flag not being handled correctly #76

Uh oh!

Conversation

vcoracolombo commented Mar 9, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vcoracolombo commented Mar 22, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vcoracolombo commented May 25, 2022

Uh oh!

Uh oh!