https://bugs.kde.org/show_bug.cgi?id=385411
--- Comment #7 from Andreas Arnez <ar...@linux.ibm.com> ---
(In reply to Vadim Barkov from comment #6)
> Good news! Is any additional work needed on this issue?
As I said before, long lines should be avoided. And there are a few other nits
in the implementation (attachment 115427):
> Subject: [PATCH 2/2] Vector floating point implementation (code)
> [...]
> --- a/VEX/priv/guest_s390_toIR.c
> +++ b/VEX/priv/guest_s390_toIR.c
> [...]
> return "vmalh";
> }
>
> +static void
> +s390_vector_fp_from_or_to_operation(IROp op, IRType fromType, IRType toTy…
Very long line; please split appropriately. It may also help to shorten
the function name to something like "s390_vector_fp_convert". I think
this would improve readability of the function's invocations as well.
Source lines in Valgrind should generally not exceed 80 columns. You can
find long lines in your patch with a grep command like this:
grep -nH '^+ .\{81\}' my.patch
> [...]
> + for Iop_F64toF32 we do this:
> + f64[0] -> f32[0]
> + f64[1] -> f32[2]
> +
> + The magic below with scaling factors is used to achive the logic de…
Replace "achive" by "achieve".
> [...]
> + vassert(m3 == 3);
> +
> + IRExpr* result;
> + switch (m5) {
> + case 0: {
Space at end of line.
> [...]
> + get_vr_qw(v3));
> + result = triop(addOrSub,
> + irrm,
> + mulResult,
> + get_vr_qw(v4));
You should probably add a comment here, explaining that the separate
multiply and add/sub operations are combined to a fused multiply-add/sub
by s390_isel_vec_expr_wrk() in host_s390_isel.c.
> [...]
> + if (!s390_vr_is_cs_set(m6)) {
> + if (LIKELY(!isSingleElementOp)) {
> + put_vr_qw(v1, binop(Iop_CmpEQ64Fx2, get_vr_qw(v2), get_vr_qw(v3)…
> + } else {
> + IRExpr* comparationResult = binop(Iop_CmpF64, get_vr(v2, Ity_F64…
Replace "comparation" by "comparison". Same in s390_irgen_VFCH and
s390_irgen_VFCHE.
> [...]
> --- a/VEX/priv/host_s390_isel.c
> +++ b/VEX/priv/host_s390_isel.c
> [...]
>
> + Iop_irrm_V_wrk: {
> + set_bfp_rounding_mode_in_fpc(env, arg1);
> + reg1 = s390_isel_vec_expr(env, arg2);
> +
Spaces at end of line.
> [...]
> + case Iop_Add64Fx2:
> + size = 8;
> +
> + /* Add64Fx2(Mul64Fx2(arg1, arg2), arg3) -> MAdd(arg1, arg2, arg3…
> + if (UNLIKELY((arg2->tag == Iex_Triop)
Space at end of line.
> + && (arg2->Iex.Triop.details->op == Iop_Mul64Fx2)
> + && (arg1 == arg2->Iex.Triop.details->arg1))
> + ) {
> + vec_op = S390_VEC_FLOAT_MADD;
> + goto Iop_irrm_MAddOrSub;
OK, so this is where you combine separate multiply and add operations to a
fused multiply-add. Is it guaranteed that this logic triggers whenever
the original instruction was a multiply-add, and only then? We must not
do this if the program executes the instructions separately, because it
would change the result.
> [...]
> + case Iop_Sub64Fx2:
> + size = 8;
> +
> + /* Sub64Fx2(Mul64Fx2(arg1, arg2), arg3) -> MSub(arg1, arg2, arg3…
> + if (UNLIKELY((arg2->tag == Iex_Triop)
Space at end of line.
--
You are receiving this mail because:
You are watching all bug changes.
