Re: [Qemu-devel] [PATCH 5/6] target/mips: MXU: Add handlers for max/min instructions

[Richard Henderson <richard.henderson@linaro.org>]

Sorry I missed the original post, but:

> +    } 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 - find which one is not...*/
> +        uint32_t XRx = XRb ? XRb : XRc;
> +        /* ...and do max/min operation with one operand 0 */
> +        if (opc == OPC_MXU_S32MAX) {
> +            tcg_gen_smax_i32(mxu_gpr[XRa - 1], mxu_gpr[XRx - 1], 0);
> +        } else {
> +            tcg_gen_smin_i32(mxu_gpr[XRa - 1], mxu_gpr[XRx - 1], 0);
> +        }
> +    } else if (unlikely(XRb == XRc)) {
> +        /* both operands same -> just set destination to one of them */
> +        tcg_gen_mov_i32(mxu_gpr[XRa - 1], mxu_gpr[XRb - 1]);

You should not special case unlikely events, especially when ...

> +    } else {
> +        /* the most general case */
> +        if (opc == OPC_MXU_S32MAX) {
> +            tcg_gen_smax_i32(mxu_gpr[XRa - 1], mxu_gpr[XRb - 1],
> +                                               mxu_gpr[XRc - 1]);
> +        } else {
> +            tcg_gen_smin_i32(mxu_gpr[XRa - 1], mxu_gpr[XRb - 1],
> +                                               mxu_gpr[XRc - 1]);
> +        }

... the normal case will handle those special cases just fine.