On 3/15/21 10:33 PM, Peter Maydell wrote:
> 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...
Oops, our mails crossed :)
I wonder if this is a simple rescan or if target/mips/translate.c
is too big and Coverity bails out on it by timeout (this is what
Coccinelle does). Now the extracted code could finally get processed.
>> +/*
>> + * 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.
Yes, from the spec XRa is the destination register, XRb/XRc are
the compared inputs. Unfortunately I couldn't sort the function
body code so I ended rewriting it.
Regards,
Phil.
