From mboxrd@z Thu Jan  1 00:00:00 1970
Received: from eggs.gnu.org ([208.118.235.92]:43300)
	by lists.gnu.org with esmtp (Exim 4.71)
	(envelope-from <rth7680@gmail.com>) id 1TLfG3-0006QG-Oe
	for qemu-devel@nongnu.org; Tue, 09 Oct 2012 15:14:24 -0400
Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)
	(envelope-from <rth7680@gmail.com>) id 1TLfFz-0005gp-96
	for qemu-devel@nongnu.org; Tue, 09 Oct 2012 15:14:23 -0400
Received: from mail-pa0-f45.google.com ([209.85.220.45]:46109)
	by eggs.gnu.org with esmtp (Exim 4.71)
	(envelope-from <rth7680@gmail.com>) id 1TLfFz-0005gY-0V
	for qemu-devel@nongnu.org; Tue, 09 Oct 2012 15:14:19 -0400
Received: by mail-pa0-f45.google.com with SMTP id fb10so5529302pad.4
	for <qemu-devel@nongnu.org>; Tue, 09 Oct 2012 12:14:18 -0700 (PDT)
Sender: Richard Henderson <rth7680@gmail.com>
Message-ID: <50747788.1080505@twiddle.net>
Date: Tue, 09 Oct 2012 12:14:16 -0700
From: Richard Henderson <rth@twiddle.net>
MIME-Version: 1.0
References: <1349526621-13939-1-git-send-email-pbonzini@redhat.com>
	<1349526621-13939-9-git-send-email-pbonzini@redhat.com>
In-Reply-To: <1349526621-13939-9-git-send-email-pbonzini@redhat.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Subject: Re: [Qemu-devel] [PATCH 08/14] i386: do not compute eflags multiple
 times consecutively
List-Id: <qemu-devel.nongnu.org>
List-Unsubscribe: <https://lists.nongnu.org/mailman/options/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=unsubscribe>
List-Archive: <http://lists.nongnu.org/archive/html/qemu-devel>
List-Post: <mailto:qemu-devel@nongnu.org>
List-Help: <mailto:qemu-devel-request@nongnu.org?subject=help>
List-Subscribe: <https://lists.nongnu.org/mailman/listinfo/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=subscribe>
To: Paolo Bonzini <pbonzini@redhat.com>
Cc: qemu-devel@nongnu.org

On 10/06/2012 05:30 AM, Paolo Bonzini wrote:
> +static void gen_compute_eflags(DisasContext *s)
>  {
>      if (s->cc_op != CC_OP_DYNAMIC) {
>          gen_op_set_cc_op(s->cc_op);
>      }
> +    if (s->cc_op == CC_OP_EFLAGS) {
> +        return;
>      }
> +    gen_helper_cc_compute_all(cpu_tmp2_i32, cpu_env, cpu_cc_op);
> +    tcg_gen_discard_tl(cpu_cc_dst);
> +    s->cc_op = CC_OP_EFLAGS;
> +    tcg_gen_extu_i32_tl(cpu_cc_src, cpu_tmp2_i32);
>  }

Can we at this point in the series assert that if s->cc_op == CC_OP_EFLAGS,
then cpu_cc_op has also been assigned CC_OP_EFLAGS?  If so, then we can do

    if (s->cc_op == CC_OP_EFLAGS) {
        return;
    }
    if (s->cc_op != CC_OP_DYNAMIC) {
        gen_op_set_cc_op(s->cc_op);
    }
    ...

As-is it would appear that we get redundant assignments to cpu_cc_op when
calling this routine twice in a row.  And with that helper call in between
we won't be able to eliminate the second assignment.

I'll also note that we'd probably get better code if gen_helper_cc_compute_all
took all of cpu_cc_{op,src,dst} as arguments so that it could be CONST+PURE.
With just that changed I think the redundant assignment to cpu_cc_op would
be eliminated.

All that said, I don't see anything wrong with the patch as-is, and probably
these other things I mention would want to be follow-on patches anyway.

Reviewed-by: Richard Henderson <rth@twiddle.net>


r~