Re: [PULL 18/27] target/mips: Extract MXU code to new mxu_translate.c file

[Peter Maydell <peter.maydell@linaro.org>]

On Sat, 13 Mar 2021 at 19:58, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>
> Extract 1600+ lines from the big translate.c into a new file.
>
> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@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