Re: [Qemu-devel] [PULL 21/26] target-hppa: Implement branches

[Paolo Bonzini]

I haven't studied the code much, so I'm just reporting what Coverity
says.  Not sure if the code has a bug or can just be simplified.

On 23/01/2017 03:17, Richard Henderson wrote:
> +/* Emit a conditional branch to a direct target.  If the branch itself
> +   is nullified, we should have already used nullify_over.  */
> +static ExitStatus do_cbranch(DisasContext *ctx, target_long disp, bool is_n,
> +                             DisasCond *cond)
> +{
> +    target_ulong dest = iaoq_dest(ctx, disp);
> +    TCGLabel *taken = NULL;
> +    TCGCond c = cond->c;
> +    int which = 0;
> +    bool n;
> +
> +    assert(ctx->null_cond.c == TCG_COND_NEVER);
> +
> +    /* Handle TRUE and NEVER as direct branches.  */
> +    if (c == TCG_COND_ALWAYS) {
> +        return do_dbranch(ctx, dest, 0, is_n && disp >= 0);
> +    }
> +    if (c == TCG_COND_NEVER) {
> +        return do_dbranch(ctx, ctx->iaoq_n, 0, is_n && disp < 0);
> +    }
> +
> +    taken = gen_new_label();
> +    cond_prep(cond);
> +    tcg_gen_brcond_tl(c, cond->a0, cond->a1, taken);
> +    cond_free(cond);
> +
> +    /* Not taken: Condition not satisfied; nullify on backward branches. */
> +    n = is_n && disp < 0;
> +    if (n && use_nullify_skip(ctx)) {
> +        nullify_set(ctx, 0);
> +        gen_goto_tb(ctx, which++, ctx->iaoq_n, ctx->iaoq_n + 4);
> +    } else {
> +        if (!n && ctx->null_lab) {
> +            gen_set_label(ctx->null_lab);
> +            ctx->null_lab = NULL;
> +        }
> +        nullify_set(ctx, n);
> +        gen_goto_tb(ctx, which++, ctx->iaoq_b, ctx->iaoq_n);
> +    }

Both branches increment "which", so you can replace it with 0 and which
is always 1 now.

> +    gen_set_label(taken);
> +
> +    /* Taken: Condition satisfied; nullify on forward branches.  */
> +    n = is_n && disp >= 0;
> +    if (n && use_nullify_skip(ctx)) {
> +        nullify_set(ctx, 0);
> +        gen_goto_tb(ctx, which++, dest, dest + 4);
> +    } else {
> +        nullify_set(ctx, n);
> +        gen_goto_tb(ctx, which++, ctx->iaoq_b, dest);
> +    }

Both branches increment "which", so you can replace it with 1 and which
is always 2.

> +    /* Not taken: the branch itself was nullified.  */
> +    if (ctx->null_lab) {
> +        gen_set_label(ctx->null_lab);
> +        ctx->null_lab = NULL;
> +        if (which < 2) {
> +            nullify_set(ctx, 0);
> +            gen_goto_tb(ctx, which, ctx->iaoq_b, ctx->iaoq_n);
> +            return EXIT_GOTO_TB;

So this branch of the "if" is dead.

> +        } else {
> +            return EXIT_IAQ_N_STALE;
> +        }
> +    } else {
> +        return EXIT_GOTO_TB;
> +    }
> +}