On Sat, 13 Mar 2021 at 19:58, Philippe Mathieu-Daudé <[email protected]> wrote:
>
> Extract 1600+ lines from the big translate.c into a new file.
>
> Reviewed-by: Richard Henderson <[email protected]>
> Signed-off-by: Philippe Mathieu-Daudé <[email protected]>
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
