diff mbox

Shifts, ppc[64], xtensa

Message ID CAMo8Bf+eOWuyPfrN6W4aWr9FNpoBU+M7FC1AoOD49_O2G1h1Gw@mail.gmail.com
State New
Headers show

Commit Message

Max Filippov Sept. 20, 2012, 12:29 a.m. UTC 
On Wed, Sep 19, 2012 at 11:53 PM, Richard Henderson <rth@twiddle.net> wrote:
> On 09/19/2012 11:30 AM, Peter Maydell wrote:
>> ...but on the other hand that ought to work for PPC too, so
>> presumably my analysis is wrong somewhere.
>
> It isn't.  It's a target-xtensa bug.
>
>> OP:
>>  ---- 0xd0079cff
>>  movi_i32 tmp0,$0xd0079cff
>>  movi_i32 tmp1,$0x2
>>  movi_i32 tmp2,$0x1
>>  movi_i32 tmp3,$advance_ccount
>>  call tmp3,$0x0,$0,env,tmp2
>>  movi_i32 tmp2,$window_check
>>  call tmp2,$0x0,$0,env,tmp0,tmp1
>>  and_i32 tmp0,ar9,ar8
>>  movi_i32 tmp1,$0x0
>>  brcond_i32 tmp0,tmp1,eq,$0x0
>>  movi_i32 tmp2,$0x0
>>  brcond_i32 LCOUNT,tmp2,eq,$0x1
>>  movi_i32 tmp2,$0x1
>>  sub_i32 LCOUNT,LCOUNT,tmp2
>>  movi_i32 tmp2,$0xd0079cf2
>>  mov_i32 pc,tmp2
>>  goto_tb $0x0
>>  exit_tb $0x4a116558
>>  set_label $0x1
>>  movi_i32 tmp2,$0xd0079d02
>>  mov_i32 pc,tmp2
>>  exit_tb $0x0
>>  set_label $0x0
>>  movi_i32 tmp2,$0xd0079d1a
>>  mov_i32 pc,tmp2
>>  goto_tb $0x1
>>  exit_tb $0x4a116559
>>  movi_i32 tmp0,$0x0
>>  brcond_i32 LCOUNT,tmp0,eq,$0x2
>>  movi_i32 tmp0,$0x1
>>  sub_i32 LCOUNT,LCOUNT,tmp0
>>  movi_i32 tmp0,$0xd0079cf2
>>  mov_i32 pc,tmp0
>>  goto_tb $0x0
>>  exit_tb $0x4a116558
>>  set_label $0x2
>>  movi_i32 tmp0,$0xd0079d02
>>  mov_i32 pc,tmp0
>>  exit_tb $0x0
>
> There are two instances of goto_tb $0 in here.
> And, amusingly, two checks for LCOUNT.
>
> Since there's no disassembler for xtensa, I'll
> leave it to the maintainer to track down from
> whence this mistake stems.

This code is generated when TB ends on LEND
(zero-overhead loop ending) with branching instruction.
So, in addition to 3 way branch there's extra looping code
generated by unconditional gen_check_loop_end(dc, 0);
at the end of disas_xtensa_insn. I was pretty sure that this
dead code would make no harm. --enable-debug-tcg says
nothing on x86_64 host. The following should fix that:

-- >8 --
From: Max Filippov <jcmvbkbc@gmail.com>
Date: Thu, 20 Sep 2012 04:07:20 +0400
Subject: [PATCH] target-xtensa: don't emit extra gen_check_loop_end

Unconditional gen_check_loop_end at the end of disas_xtensa_insn
can emit tcg_gen_goto_tb with slot id already used in the TB (e.g. when
TB ends at LEND with a branch). Not all tcg backends can handle that.

Signed-off-by: Max Filippov <jcmvbkbc@gmail.com>
---
 target-xtensa/translate.c |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)

Comments

Richard Henderson Sept. 20, 2012, 2:03 p.m. UTC | #1 
On 09/19/2012 05:29 PM, Max Filippov wrote:
> Not all tcg backends can handle that.

*No* tcg backends can handle that.

If we fix the bug wherein i386 clobbers the goto_tb target
during re-translation you'll see the crash there too.

And we were just talking about enhancing --enable-debug-tcg
to detect this case...


r~
Max Filippov Sept. 20, 2012, 2:22 p.m. UTC | #2 
On Thu, Sep 20, 2012 at 6:03 PM, Richard Henderson <rth@twiddle.net> wrote:
> On 09/19/2012 05:29 PM, Max Filippov wrote:
>> Not all tcg backends can handle that.
>
> *No* tcg backends can handle that.
>
> If we fix the bug wherein i386 clobbers the goto_tb target
> during re-translation you'll see the crash there too.

Ok, I will document it in the places that I know.

> And we were just talking about enhancing --enable-debug-tcg
> to detect this case...

I can try to do it too as my homework (:
malc Sept. 20, 2012, 10:53 p.m. UTC | #3 
On Thu, 20 Sep 2012, Max Filippov wrote:

> On Wed, Sep 19, 2012 at 11:53 PM, Richard Henderson <rth@twiddle.net> wrote:
> > On 09/19/2012 11:30 AM, Peter Maydell wrote:
> >> ...but on the other hand that ought to work for PPC too, so
> >> presumably my analysis is wrong somewhere.
> >
> > It isn't.  It's a target-xtensa bug.
> >

[..snip..]

> 
> This code is generated when TB ends on LEND
> (zero-overhead loop ending) with branching instruction.
> So, in addition to 3 way branch there's extra looping code
> generated by unconditional gen_check_loop_end(dc, 0);
> at the end of disas_xtensa_insn. I was pretty sure that this
> dead code would make no harm. --enable-debug-tcg says
> nothing on x86_64 host. The following should fix that:

It does, once the commit message is fixed i can apply it.

> 
> -- >8 --
> From: Max Filippov <jcmvbkbc@gmail.com>
> Date: Thu, 20 Sep 2012 04:07:20 +0400
> Subject: [PATCH] target-xtensa: don't emit extra gen_check_loop_end
> 
> Unconditional gen_check_loop_end at the end of disas_xtensa_insn
> can emit tcg_gen_goto_tb with slot id already used in the TB (e.g. when
> TB ends at LEND with a branch). Not all tcg backends can handle that.
> 
> Signed-off-by: Max Filippov <jcmvbkbc@gmail.com>
> ---
>  target-xtensa/translate.c |    4 +++-
>  1 files changed, 3 insertions(+), 1 deletions(-)
> 
> diff --git a/target-xtensa/translate.c b/target-xtensa/translate.c
> index 63b37b3..57a2b6f 100644
> --- a/target-xtensa/translate.c
> +++ b/target-xtensa/translate.c
> @@ -2502,7 +2502,9 @@ static void disas_xtensa_insn(DisasContext *dc)
>          break;
>      }
> 
> -    gen_check_loop_end(dc, 0);
> +    if (dc->is_jmp == DISAS_NEXT) {
> +        gen_check_loop_end(dc, 0);
> +    }
>      dc->pc = dc->next_pc;
> 
>      return;
>
diff mbox

Patch

diff --git a/target-xtensa/translate.c b/target-xtensa/translate.c
index 63b37b3..57a2b6f 100644
--- a/target-xtensa/translate.c
+++ b/target-xtensa/translate.c
@@ -2502,7 +2502,9 @@  static void disas_xtensa_insn(DisasContext *dc)
         break;
     }

-    gen_check_loop_end(dc, 0);
+    if (dc->is_jmp == DISAS_NEXT) {
+        gen_check_loop_end(dc, 0);
+    }
     dc->pc = dc->next_pc;

     return;