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

[Philippe Mathieu-Daudé]

On Tue, Dec 25, 2018 at 8:35 PM Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> 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.

Also because we have now 3 Coverity CID:

 *** CID 1450831:    (OVERRUN)
in gen_mxu_D16MAX_D16MIN()
1107             TCGv_i32 t0 = tcg_temp_new();
1108             TCGv_i32 t1 = tcg_const_i32(0);
1109
1110             /* the left half-word first */
1111             tcg_gen_andi_i32(t0, mxu_gpr[XRx - 1], 0xFFFF0000);
1112             if (opc == OPC_MXU_D16MAX) {
>>>     CID 1450831:    (OVERRUN)
>>>     Overrunning array "mxu_gpr" of 15 8-byte elements at element index 
>>> 4294967295 (byte offset 34359738367) using index "XRa - 1U" (which 
>>> evaluates to 4294967295).
1113                 tcg_gen_smax_i32(mxu_gpr[XRa - 1], t0, t1);

>>>     CID 1450831:    (OVERRUN)
>>>     Overrunning array "mxu_gpr" of 15 8-byte elements at element index 
>>> 4294967295 (byte offset 34359738367) using index "XRa - 1U" (which 
>>> evaluates to 4294967295).
1114             } else {
1115                 tcg_gen_smin_i32(mxu_gpr[XRa - 1], t0, t1);
1116             }
1117
1118             /* the right half-word */
1119             tcg_gen_andi_i32(t0, mxu_gpr[XRx - 1], 0x0000FFFF);

1125             } else {
1126                 tcg_gen_smin_i32(t0, t0, t1);
1127             }
1128             /* return resulting half-words to its original position */
1129             tcg_gen_shri_i32(t0, t0, 16);
1130             /* finally update the destination */
>>>     CID 1450831:    (OVERRUN)
>>>     Overrunning array "mxu_gpr" of 15 8-byte elements at element index 
>>> 4294967295 (byte offset 34359738367) using index "XRa - 1U" (which 
>>> evaluates to 4294967295).
1131             tcg_gen_or_i32(mxu_gpr[XRa - 1], mxu_gpr[XRa - 1], t0);
1132
1133             tcg_temp_free(t1);
1134             tcg_temp_free(t0);
1135         } else if (unlikely(XRb == XRc)) {
1136             /* both operands same -> just set destination to one of them */