On 22/02/2022 21:20, Richard Henderson wrote:> On 2/22/22 04:36,
matheus.ferst@eldorado.org.br wrote:
>> From: Víctor Colombo <victor.colombo@eldorado.org.br>
>>
>> Refactor VSX_SCALAR_CMP_DP, changing its name to VSX_SCALAR_CMP and
>> prepare the helper to be used for quadword comparisons.
>>
>> Signed-off-by: Víctor Colombo <victor.colombo@eldorado.org.br>
>> Signed-off-by: Matheus Ferst <matheus.ferst@eldorado.org.br>
>> ---
>> target/ppc/fpu_helper.c | 31 ++++++++++++++-----------------
>> 1 file changed, 14 insertions(+), 17 deletions(-)
>>
>> diff --git a/target/ppc/fpu_helper.c b/target/ppc/fpu_helper.c
>> index 9b034d1fe4..5ebbcfe3b7 100644
>> --- a/target/ppc/fpu_helper.c
>> +++ b/target/ppc/fpu_helper.c
>> @@ -2265,28 +2265,30 @@ VSX_MADDQ(XSNMSUBQP, NMSUB_FLGS, 0)
>> VSX_MADDQ(XSNMSUBQPO, NMSUB_FLGS, 0)
>>
>> /*
>> - * VSX_SCALAR_CMP_DP - VSX scalar floating point compare double
>> precision
>> + * VSX_SCALAR_CMP - VSX scalar floating point compare
>> * op - instruction mnemonic
>> + * tp - type
>> * cmp - comparison operation
>> * exp - expected result of comparison
>> + * fld - vsr_t field
>> * svxvc - set VXVC bit
>> */
>> -#define VSX_SCALAR_CMP_DP(op, cmp, exp,
>> svxvc) \
>> +#define VSX_SCALAR_CMP(op, tp, cmp, fld, exp,
>> svxvc) \
>> void helper_##op(CPUPPCState *env, ppc_vsr_t
>> *xt, \
>> ppc_vsr_t *xa, ppc_vsr_t
>> *xb) \
>>
>> {
>> \
>> - ppc_vsr_t t =
>> *xt; \
>> + ppc_vsr_t t = {
>> }; \
>> bool vxsnan_flag = false, vxvc_flag = false, vex_flag =
>> false; \
>>
>> \
>> - if (float64_is_signaling_nan(xa->VsrD(0), &env->fp_status)
>> || \
>> - float64_is_signaling_nan(xb->VsrD(0), &env->fp_status))
>> { \
>> + if (tp##_is_signaling_nan(xa->fld, &env->fp_status)
>> || \
>> + tp##_is_signaling_nan(xb->fld, &env->fp_status))
>> { \
>> vxsnan_flag =
>> true; \
>> if (fpscr_ve == 0 && svxvc)
>> { \
>> vxvc_flag =
>> true; \
>>
>> } \
>> } else if (svxvc)
>> { \
>> - vxvc_flag = float64_is_quiet_nan(xa->VsrD(0),
>> &env->fp_status) || \
>> - float64_is_quiet_nan(xb->VsrD(0),
>> &env->fp_status); \
>> + vxvc_flag = tp##_is_quiet_nan(xa->fld, &env->fp_status)
>> || \
>> + tp##_is_quiet_nan(xb->fld,
>> &env->fp_status); \
>> }
>
> Note that this can be simplified further, using the full FloatRelation
> result and
> float_flag_invalid_snan.
>
> Note that do_scalar_cmp gets half-way there, only checking for NaNs once
> we have
> float_relation_unordered as a comparision result. But it could go
> further and check
> float_flag_invalid_snan and drop all of the other checks vs snan and qnan.
>
>
> r~
Hello Richard! Thanks for your review
Could you please elaborate more on how do you think using
float*_compare and its FloatRelation result would work here?
I noticed do_scalar_cmp modifies CR and sets FPCC flag, which
is not what VSX_SCALAR_CMP do. Using that function would require a
rework.
An option I though would be to bring into VSX_SCALAR_CMP the
important necessary parts, something like this:
#define VSX_SCALAR_CMP(op, tp, cmp, fld, svxvc, expr)
...
r = tp##_compare(xa->fld, xb->fld, &env->fp_status);
\
if (expr) {
\
memset(&t.fld, 0xFF, sizeof(t.fld));
\
} else if (r == float_relation_unordered) {
\
if (env->fp_status.float_exception_flags &
float_flag_invalid_snan) { \
float_invalid_op_vxsnan(env, GETPC());
\
if (fpscr_ve == 0 && svxvc) {
\
float_invalid_op_vxvc(env, 0, GETPC());
\
}
\
} else if (svxvc) {
\
if (tp##_is_quiet_nan(xa->fld, &env->fp_status) ||
\
tp##_is_quiet_nan(xb->fld, &env->fp_status)) {
\
float_invalid_op_vxvc(env, 0, GETPC());
\
}
\
}
\
}
\
...
VSX_SCALAR_CMP(XSCMPEQDP, float64, eq, VsrD(0), 0, r ==
float_relation_equal)
VSX_SCALAR_CMP(XSCMPGEDP, float64, le, VsrD(0), 1, \
r == float_relation_equal || r == float_relation_greater)
VSX_SCALAR_CMP(XSCMPGTDP, float64, lt, VsrD(0), 1, r ==
float_relation_greater)
But this still looks convoluted. Another option I came with would be:
ppc_vsr_t t = { };
\
\
helper_reset_fpstatus(env);
\
\
if (tp##_##cmp##_quiet(xb->fld, xa->fld, &env->fp_status)) {
\
memset(&t.fld, 0xFF, sizeof(t.fld));
\
}
\
\
if (env->fp_status.float_exception_flags & float_flag_invalid_snan)
{ \
float_invalid_op_vxsnan(env, GETPC());
\
if (fpscr_ve == 0 && svxvc) {
\
float_invalid_op_vxvc(env, 0, GETPC());
\
}
\
} else if (svxvc) {
\
if (tp##_is_quiet_nan(xa->fld, &env->fp_status) ||
\
tp##_is_quiet_nan(xb->fld, &env->fp_status)) {
\
float_invalid_op_vxvc(env, 0, GETPC());
\
}
\
}
\
Is this close to what you were thinking?
Thank you very much!
-- Víctor