Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
vcoracolombo wants to merge 6 commits into from
Closed

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

Changes from 2 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
3f40cb2
target/ppc: Fix FPSCR.FI flag for VSX_CVT_INT_TO_FP helper
vcoracolombo Mar 7, 2022
6dd0995
target/ppc: Fix FPSCR.FI flag for VSX_MADD helper
vcoracolombo Mar 7, 2022
b276447
fixup! target/ppc: Fix FPSCR.FI flag for VSX_CVT_INT_TO_FP helper
vcoracolombo Mar 23, 2022
8c6e2a6
fixup! target/ppc: Fix FPSCR.FI flag for VSX_MADD helper
vcoracolombo Mar 23, 2022
470df3f
fixup! target/ppc: Fix FPSCR.FI flag for VSX_CVT_INT_TO_FP helper
vcoracolombo Mar 24, 2022
70ca489
fixup! fixup! target/ppc: Fix FPSCR.FI flag for VSX_MADD helper
vcoracolombo Mar 29, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
224 changes: 178 additions & 46 deletions target/ppc/fpu_helper.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -212,6 +212,24 @@ static void finish_invalid_op_excp(CPUPPCState *env, int op, uintptr_t retaddr)
}
}

/* TODO: this is not a 'finish' anymore, find a better name */
static void finish_invalid_op_excp2(CPUPPCState *env, int op)
{
/* Update the floating-point invalid operation summary */
env->fpscr |= FP_VX;
/* Update the floating-point exception summary */
env->fpscr |= FP_FX;
if (fpscr_ve != 0) {
Copy link

@alqotel alqotel Mar 22, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link

@mferst mferst Mar 22, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

/* Update the floating-point enabled exception summary */
env->fpscr |= FP_FEX;

CPUState *cs = env_cpu(env);
cs->exception_index = POWERPC_EXCP_PROGRAM;
env->error_code = POWERPC_EXCP_FP | op;
/* Exception is deferred */
}
}

static void finish_invalid_op_arith(CPUPPCState *env, int op,
bool set_fpcc, uintptr_t retaddr)
{
Expand All @@ -225,13 +243,33 @@ static void finish_invalid_op_arith(CPUPPCState *env, int op,
finish_invalid_op_excp(env, op, retaddr);
}

/* 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);
Copy link

@alqotel alqotel Mar 22, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

@vcoracolombo vcoracolombo Mar 23, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link

@mferst mferst Mar 23, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

if (fpscr_ve == 0) {
Copy link

@alqotel alqotel Mar 24, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

if (set_fpcc) {
env->fpscr &= ~FP_FPCC;
env->fpscr |= (FP_C | FP_FU);
}
}
finish_invalid_op_excp2(env, op);
}

/* Signalling NaN */
static void float_invalid_op_vxsnan(CPUPPCState *env, uintptr_t retaddr)
{
env->fpscr |= FP_VXSNAN;
finish_invalid_op_excp(env, POWERPC_EXCP_FP_VXSNAN, retaddr);
}

/* TODO: Rename this to float_invalid_op_vxsnan when conversion is over */
static void float_invalid_op_vxsnan2(CPUPPCState *env)
{
env->fpscr |= FP_VXSNAN;
finish_invalid_op_excp2(env, POWERPC_EXCP_FP_VXSNAN);
}

/* Magnitude subtraction of infinities */
static void float_invalid_op_vxisi(CPUPPCState *env, bool set_fpcc,
uintptr_t retaddr)
Expand All @@ -240,6 +278,13 @@ static void float_invalid_op_vxisi(CPUPPCState *env, bool set_fpcc,
finish_invalid_op_arith(env, POWERPC_EXCP_FP_VXISI, set_fpcc, retaddr);
}

/* TODO: Rename this to float_invalid_op_vxisi when conversion is over */
static void float_invalid_op_vxisi2(CPUPPCState *env, bool set_fpcc)
{
env->fpscr |= FP_VXISI;
finish_invalid_op_arith2(env, POWERPC_EXCP_FP_VXISI, set_fpcc);
}

/* Division of infinity by infinity */
static void float_invalid_op_vxidi(CPUPPCState *env, bool set_fpcc,
uintptr_t retaddr)
Expand All @@ -264,6 +309,13 @@ static void float_invalid_op_vximz(CPUPPCState *env, bool set_fpcc,
finish_invalid_op_arith(env, POWERPC_EXCP_FP_VXIMZ, set_fpcc, retaddr);
}

/* TODO: Rename this to float_invalid_op_vximz when conversion is over */
static void float_invalid_op_vximz2(CPUPPCState *env, bool set_fpcc)
{
env->fpscr |= FP_VXIMZ;
finish_invalid_op_arith2(env, POWERPC_EXCP_FP_VXIMZ, set_fpcc);
}

/* Square root of a negative number */
static void float_invalid_op_vxsqrt(CPUPPCState *env, bool set_fpcc,
uintptr_t retaddr)
Expand Down Expand Up @@ -383,6 +435,22 @@ static inline void float_inexact_excp(CPUPPCState *env)
}
}

static inline void float_inexact_excp2(CPUPPCState *env)
Copy link
Author

@vcoracolombo vcoracolombo Mar 9, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

{
CPUState *cs = env_cpu(env);

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

@alqotel alqotel Mar 22, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

/* Update the floating-point enabled exception summary */
env->fpscr |= FP_FEX;
/* We must update the target FPR before raising the exception */
cs->exception_index = POWERPC_EXCP_PROGRAM;
env->error_code = POWERPC_EXCP_FP | POWERPC_EXCP_FP_XX;
}
}

void helper_fpscr_clrbit(CPUPPCState *env, uint32_t bit)
{
uint32_t mask = 1u << bit;
Expand Down Expand Up @@ -462,6 +530,51 @@ void helper_fpscr_check_status(CPUPPCState *env)
}
}

static void finish_fp_operation(CPUPPCState *env, int mask, uintptr_t raddr)
{
CPUState *cs = env_cpu(env);
int flags = get_float_exception_flags(&env->fp_status);

if (flags & float_flag_invalid) {
if ((mask & FP_VXIMZ) && (flags & float_flag_invalid_imz)) {
float_invalid_op_vximz2(env, mask & FP_FPRF);
}
if ((mask & FP_VXISI) && (flags & float_flag_invalid_isi)) {
float_invalid_op_vxisi2(env, mask & FP_FPRF);
}
if ((mask & FP_VXSNAN) && (flags & float_flag_invalid_snan)) {
float_invalid_op_vxsnan2(env);
}

/* TODO: Add other invalid exceptions */
}

if ((mask & FP_OX) && (flags & float_flag_overflow)) {
float_overflow_excp(env);
}
if ((mask & FP_UX) && (flags & float_flag_underflow)) {
float_underflow_excp(env);
}
if (mask & FP_FI) {
if (flags & float_flag_inexact) {
env->fpscr |= FP_FI;
} else {
env->fpscr &= ~FP_FI;
}
}
if ((mask & FP_XX) && (flags & float_flag_inexact)) {
float_inexact_excp2(env);
}

if (cs->exception_index == POWERPC_EXCP_PROGRAM &&
(env->error_code & POWERPC_EXCP_FP)) {
if (fp_exceptions_enabled(env)) {
raise_exception_err_ra(env, cs->exception_index,
env->error_code, raddr);
}
}
}

static void do_float_check_status(CPUPPCState *env, uintptr_t raddr)
{
CPUState *cs = env_cpu(env);
Expand Down Expand Up @@ -2155,9 +2268,8 @@ VSX_TSQRT(xvtsqrtsp, 4, float32, VsrW(i), -126, 23)
* fld - vsr_t field (VsrD(*) or VsrW(*))
* maddflgs - flags for the float*muladd routine that control the
* various forms (madd, msub, nmadd, nmsub)
* sfprf - set FPRF
*/
#define VSX_MADD(op, nels, tp, fld, maddflgs, sfprf) \
#define VSX_MADD(op, nels, tp, fld, maddflgs, fpscr_mask) \
void helper_##op(CPUPPCState *env, ppc_vsr_t *xt, \
ppc_vsr_t *s1, ppc_vsr_t *s2, ppc_vsr_t *s3) \
{ \
Expand All @@ -2172,37 +2284,49 @@ void helper_##op(CPUPPCState *env, ppc_vsr_t *xt, \
t.fld = tp##_muladd(s1->fld, s3->fld, s2->fld, maddflgs, &tstat); \
env->fp_status.float_exception_flags |= tstat.float_exception_flags; \
\
if (unlikely(tstat.float_exception_flags & float_flag_invalid)) { \
float_invalid_op_madd(env, tstat.float_exception_flags, \
sfprf, GETPC()); \
} \
\
if (sfprf) { \
if (fpscr_mask & FP_FPRF) { \
helper_compute_fprf_float64(env, t.fld); \
} \
} \
\
*xt = t; \
do_float_check_status(env, GETPC()); \
}

VSX_MADD(XSMADDDP, 1, float64, VsrD(0), MADD_FLGS, 1)
VSX_MADD(XSMSUBDP, 1, float64, VsrD(0), MSUB_FLGS, 1)
VSX_MADD(XSNMADDDP, 1, float64, VsrD(0), NMADD_FLGS, 1)
VSX_MADD(XSNMSUBDP, 1, float64, VsrD(0), NMSUB_FLGS, 1)
VSX_MADD(XSMADDSP, 1, float64r32, VsrD(0), MADD_FLGS, 1)
VSX_MADD(XSMSUBSP, 1, float64r32, VsrD(0), MSUB_FLGS, 1)
VSX_MADD(XSNMADDSP, 1, float64r32, VsrD(0), NMADD_FLGS, 1)
VSX_MADD(XSNMSUBSP, 1, float64r32, VsrD(0), NMSUB_FLGS, 1)

VSX_MADD(xvmadddp, 2, float64, VsrD(i), MADD_FLGS, 0)
VSX_MADD(xvmsubdp, 2, float64, VsrD(i), MSUB_FLGS, 0)
VSX_MADD(xvnmadddp, 2, float64, VsrD(i), NMADD_FLGS, 0)
VSX_MADD(xvnmsubdp, 2, float64, VsrD(i), NMSUB_FLGS, 0)

VSX_MADD(xvmaddsp, 4, float32, VsrW(i), MADD_FLGS, 0)
VSX_MADD(xvmsubsp, 4, float32, VsrW(i), MSUB_FLGS, 0)
VSX_MADD(xvnmaddsp, 4, float32, VsrW(i), NMADD_FLGS, 0)
VSX_MADD(xvnmsubsp, 4, float32, VsrW(i), NMSUB_FLGS, 0)
finish_fp_operation(env, fpscr_mask, GETPC()); \
}

VSX_MADD(XSMADDDP, 1, float64, VsrD(0), MADD_FLGS, \
(FP_FPRF | FP_VXSNAN | FP_VXISI | FP_VXIMZ | FP_FI | FP_XX | FP_UX | FP_OX))
VSX_MADD(XSMSUBDP, 1, float64, VsrD(0), MSUB_FLGS, \
(FP_FPRF | FP_VXSNAN | FP_VXISI | FP_VXIMZ | FP_FI | FP_XX | FP_UX | FP_OX))
VSX_MADD(XSNMADDDP, 1, float64, VsrD(0), NMADD_FLGS, \
(FP_FPRF | FP_VXSNAN | FP_VXISI | FP_VXIMZ | FP_FI | FP_XX | FP_UX | FP_OX))
VSX_MADD(XSNMSUBDP, 1, float64, VsrD(0), NMSUB_FLGS, \
(FP_FPRF | FP_VXSNAN | FP_VXISI | FP_VXIMZ | FP_FI | FP_XX | FP_UX | FP_OX))
VSX_MADD(XSMADDSP, 1, float64r32, VsrD(0), MADD_FLGS, \
(FP_FPRF | FP_VXSNAN | FP_VXISI | FP_VXIMZ | FP_FI | FP_XX | FP_UX | FP_OX))
VSX_MADD(XSMSUBSP, 1, float64r32, VsrD(0), MSUB_FLGS, \
(FP_FPRF | FP_VXSNAN | FP_VXISI | FP_VXIMZ | FP_FI | FP_XX | FP_UX | FP_OX))
VSX_MADD(XSNMADDSP, 1, float64r32, VsrD(0), NMADD_FLGS, \
(FP_FPRF | FP_VXSNAN | FP_VXISI | FP_VXIMZ | FP_FI | FP_XX | FP_UX | FP_OX))
VSX_MADD(XSNMSUBSP, 1, float64r32, VsrD(0), NMSUB_FLGS, \
(FP_FPRF | FP_VXSNAN | FP_VXISI | FP_VXIMZ | FP_FI | FP_XX | FP_UX | FP_OX))

VSX_MADD(xvmadddp, 2, float64, VsrD(i), MADD_FLGS, \
(FP_VXSNAN | FP_VXISI | FP_VXIMZ | FP_XX | FP_UX | FP_OX))
VSX_MADD(xvmsubdp, 2, float64, VsrD(i), MSUB_FLGS, \
(FP_VXSNAN | FP_VXISI | FP_VXIMZ | FP_XX | FP_UX | FP_OX))
VSX_MADD(xvnmadddp, 2, float64, VsrD(i), NMADD_FLGS, \
(FP_VXSNAN | FP_VXISI | FP_VXIMZ | FP_XX | FP_UX | FP_OX))
VSX_MADD(xvnmsubdp, 2, float64, VsrD(i), NMSUB_FLGS, \
(FP_VXSNAN | FP_VXISI | FP_VXIMZ | FP_XX | FP_UX | FP_OX))

VSX_MADD(xvmaddsp, 4, float32, VsrW(i), MADD_FLGS, \
(FP_VXSNAN | FP_VXISI | FP_VXIMZ | FP_XX | FP_UX | FP_OX))
VSX_MADD(xvmsubsp, 4, float32, VsrW(i), MSUB_FLGS, \
(FP_VXSNAN | FP_VXISI | FP_VXIMZ | FP_XX | FP_UX | FP_OX))
VSX_MADD(xvnmaddsp, 4, float32, VsrW(i), NMADD_FLGS, \
(FP_VXSNAN | FP_VXISI | FP_VXIMZ | FP_XX | FP_UX | FP_OX))
VSX_MADD(xvnmsubsp, 4, float32, VsrW(i), NMSUB_FLGS, \
(FP_VXSNAN | FP_XX | FP_UX | FP_OX))

/*
* VSX_MADDQ - VSX floating point quad-precision muliply/add
Expand Down Expand Up @@ -3006,10 +3130,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
*/
Copy link
Author

@vcoracolombo vcoracolombo Mar 9, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

#define VSX_CVT_INT_TO_FP(op, nels, stp, ttp, sfld, tfld, sfprf, r2sp) \
#define VSX_CVT_INT_TO_FP(op, nels, stp, ttp, sfld, tfld, r2sp, fpscr_mask) \
void helper_##op(CPUPPCState *env, ppc_vsr_t *xt, ppc_vsr_t *xb) \
{ \
ppc_vsr_t t = { }; \
Expand All @@ -3020,25 +3142,35 @@ void helper_##op(CPUPPCState *env, ppc_vsr_t *xt, ppc_vsr_t *xb) \
if (r2sp) { \
t.tfld = do_frsp(env, t.tfld, GETPC()); \
} \
if (sfprf) { \
if (fpscr_mask & FP_FPRF) { \
helper_compute_fprf_float64(env, t.tfld); \
} \
} \
\
*xt = t; \
do_float_check_status(env, GETPC()); \
}

VSX_CVT_INT_TO_FP(xscvsxddp, 1, int64, float64, VsrD(0), VsrD(0), 1, 0)
VSX_CVT_INT_TO_FP(xscvuxddp, 1, uint64, float64, VsrD(0), VsrD(0), 1, 0)
VSX_CVT_INT_TO_FP(xscvsxdsp, 1, int64, float64, VsrD(0), VsrD(0), 1, 1)
VSX_CVT_INT_TO_FP(xscvuxdsp, 1, uint64, float64, VsrD(0), VsrD(0), 1, 1)
VSX_CVT_INT_TO_FP(xvcvsxddp, 2, int64, float64, VsrD(i), VsrD(i), 0, 0)
VSX_CVT_INT_TO_FP(xvcvuxddp, 2, uint64, float64, VsrD(i), VsrD(i), 0, 0)
VSX_CVT_INT_TO_FP(xvcvsxwdp, 2, int32, float64, VsrW(2 * i), VsrD(i), 0, 0)
VSX_CVT_INT_TO_FP(xvcvuxwdp, 2, uint64, float64, VsrW(2 * i), VsrD(i), 0, 0)
VSX_CVT_INT_TO_FP(xvcvsxwsp, 4, int32, float32, VsrW(i), VsrW(i), 0, 0)
VSX_CVT_INT_TO_FP(xvcvuxwsp, 4, uint32, float32, VsrW(i), VsrW(i), 0, 0)
finish_fp_operation(env, fpscr_mask, GETPC()); \
}

VSX_CVT_INT_TO_FP(xscvsxddp, 1, int64, float64, VsrD(0), VsrD(0), 0, \
(FP_FPRF | FP_FI | FP_XX))
VSX_CVT_INT_TO_FP(xscvuxddp, 1, uint64, float64, VsrD(0), VsrD(0), 0, \
(FP_FPRF | FP_FI | FP_XX))
VSX_CVT_INT_TO_FP(xscvsxdsp, 1, int64, float64, VsrD(0), VsrD(0), 1, \
(FP_FPRF | FP_FI | FP_XX))
VSX_CVT_INT_TO_FP(xscvuxdsp, 1, uint64, float64, VsrD(0), VsrD(0), 1, \
(FP_FPRF | FP_FI | FP_XX))
VSX_CVT_INT_TO_FP(xvcvsxddp, 2, int64, float64, VsrD(i), VsrD(i), 0, \
FP_XX)
VSX_CVT_INT_TO_FP(xvcvuxddp, 2, uint64, float64, VsrD(i), VsrD(i), 0, \
FP_XX)
VSX_CVT_INT_TO_FP(xvcvsxwdp, 2, int32, float64, VsrW(2 * i), VsrD(i), 0, \
FP_XX)
VSX_CVT_INT_TO_FP(xvcvuxwdp, 2, uint64, float64, VsrW(2 * i), VsrD(i), 0, \
FP_XX)
VSX_CVT_INT_TO_FP(xvcvsxwsp, 4, int32, float32, VsrW(i), VsrW(i), 0, \
FP_XX)
VSX_CVT_INT_TO_FP(xvcvuxwsp, 4, uint32, float32, VsrW(i), VsrW(i), 0, \
FP_XX)

#define VSX_CVT_INT_TO_FP2(op, stp, ttp) \
void helper_##op(CPUPPCState *env, ppc_vsr_t *xt, ppc_vsr_t *xb) \
Expand Down