On Sat, 13 Mar 2021 at 19:58, Philippe Mathieu-Daudé <f4...@amsat.org> wrote: > > Extract 1600+ lines from the big translate.c into a new file. > > Reviewed-by: Richard Henderson <richard.hender...@linaro.org> > Signed-off-by: Philippe Mathieu-Daudé <f4...@amsat.org>
This code motion caused Coverity to rescan this code, and it thinks there's a problem in this function (CID 1450831). It looks to me like it might be right... > +/* > + * D16MAX > + * Update XRa with the 16-bit-wise maximums of signed integers > + * contained in XRb and XRc. > + * > + * D16MIN > + * Update XRa with the 16-bit-wise minimums of signed integers > + * contained in XRb and XRc. > + */ > +static void gen_mxu_D16MAX_D16MIN(DisasContext *ctx) > +{ > + uint32_t pad, opc, XRc, XRb, XRa; > + > + pad = extract32(ctx->opcode, 21, 5); > + opc = extract32(ctx->opcode, 18, 3); > + XRc = extract32(ctx->opcode, 14, 4); > + XRb = extract32(ctx->opcode, 10, 4); > + XRa = extract32(ctx->opcode, 6, 4); > + > + if (unlikely(pad != 0)) { > + /* opcode padding incorrect -> do nothing */ > + } else if (unlikely(XRc == 0)) { > + /* destination is zero register -> do nothing */ > + } else if (unlikely((XRb == 0) && (XRa == 0))) { > + /* both operands zero registers -> just set destination to zero */ > + tcg_gen_movi_i32(mxu_gpr[XRc - 1], 0); > + } else if (unlikely((XRb == 0) || (XRa == 0))) { In this block of code either XRb or XRa is zero... > + /* exactly one operand is zero register - find which one is not...*/ > + uint32_t XRx = XRb ? XRb : XRc; > + /* ...and do half-word-wise max/min with one operand 0 */ > + TCGv_i32 t0 = tcg_temp_new(); > + TCGv_i32 t1 = tcg_const_i32(0); > + > + /* the left half-word first */ > + tcg_gen_andi_i32(t0, mxu_gpr[XRx - 1], 0xFFFF0000); > + if (opc == OPC_MXU_D16MAX) { > + tcg_gen_smax_i32(mxu_gpr[XRa - 1], t0, t1); > + } else { > + tcg_gen_smin_i32(mxu_gpr[XRa - 1], t0, t1); > + } but in these smax/smin calls we're clearly assuming that XRa is not zero. There seems to be some confusion over which registers are the inputs and which is the output. The top-level function comment says XRa is the input and XRb/XRc the inputs. But the "destination is zero register" comment is against a check on XRc, and the "both operands zero" comment is against a check on XRa and XRb, as is the "one operand is zero" comment... > +/* > + * Q8MAX > + * Update XRa with the 8-bit-wise maximums of signed integers > + * contained in XRb and XRc. > + * > + * Q8MIN > + * Update XRa with the 8-bit-wise minimums of signed integers > + * contained in XRb and XRc. > + */ > +static void gen_mxu_Q8MAX_Q8MIN(DisasContext *ctx) > +{ > + uint32_t pad, opc, XRc, XRb, XRa; > + > + pad = extract32(ctx->opcode, 21, 5); > + opc = extract32(ctx->opcode, 18, 3); > + XRc = extract32(ctx->opcode, 14, 4); > + XRb = extract32(ctx->opcode, 10, 4); > + XRa = extract32(ctx->opcode, 6, 4); > + > + if (unlikely(pad != 0)) { > + /* opcode padding incorrect -> do nothing */ > + } else if (unlikely(XRa == 0)) { > + /* destination is zero register -> do nothing */ > + } else if (unlikely((XRb == 0) && (XRc == 0))) { > + /* both operands zero registers -> just set destination to zero */ > + tcg_gen_movi_i32(mxu_gpr[XRa - 1], 0); > + } else if (unlikely((XRb == 0) || (XRc == 0))) { > + /* exactly one operand is zero register - make it be the first...*/ > + uint32_t XRx = XRb ? XRb : XRc; Contrast this function, where the code and the comments are all in agreement that XRa is destination and XRb/XRc inputs. thanks -- PMM