Patchwork target-xtensa: make sar_m32 global instead of local temp

login
register
mail settings
Submitter Max Filippov
Date Nov. 24, 2012, 12:51 a.m.
Message ID <1353718296-3884-1-git-send-email-jcmvbkbc@gmail.com>
Download mbox | patch
Permalink /patch/201433/
State New
Headers show

Comments

Max Filippov - Nov. 24, 2012, 12:51 a.m.
This fixes the following assertion caused by local temp reaching the end
of TB in discarded state:

  qemu-system-xtensa: tcg/tcg.c:1665: temp_save: Assertion `s->temps[temp].val_type == 2 || s->temps[temp].fixed_reg' failed.
  Aborted

Signed-off-by: Max Filippov <jcmvbkbc@gmail.com>
---
 target-xtensa/cpu.h       |    1 +
 target-xtensa/translate.c |   28 ++++++++--------------------
 2 files changed, 9 insertions(+), 20 deletions(-)
Aurelien Jarno - Nov. 24, 2012, 10:59 a.m.
On Sat, Nov 24, 2012 at 04:51:36AM +0400, Max Filippov wrote:
> This fixes the following assertion caused by local temp reaching the end
> of TB in discarded state:
> 
>   qemu-system-xtensa: tcg/tcg.c:1665: temp_save: Assertion `s->temps[temp].val_type == 2 || s->temps[temp].fixed_reg' failed.
>   Aborted
> 
> Signed-off-by: Max Filippov <jcmvbkbc@gmail.com>
> ---
>  target-xtensa/cpu.h       |    1 +
>  target-xtensa/translate.c |   28 ++++++++--------------------
>  2 files changed, 9 insertions(+), 20 deletions(-)

I have just send a patch to fix the issue in the TCG code instead (sorry
for being so long, the last weeks have been quite busy). I think it is
better than fixing the issue in target-xtensa.

If it works for you, I'll commit it.

> diff --git a/target-xtensa/cpu.h b/target-xtensa/cpu.h
> index 74e9888..f021a9a 100644
> --- a/target-xtensa/cpu.h
> +++ b/target-xtensa/cpu.h
> @@ -323,6 +323,7 @@ typedef struct CPUXtensaState {
>      const XtensaConfig *config;
>      uint32_t regs[16];
>      uint32_t pc;
> +    uint32_t sar_m32;
>      uint32_t sregs[256];
>      uint32_t uregs[256];
>      uint32_t phys_regs[MAX_NAREG];
> diff --git a/target-xtensa/translate.c b/target-xtensa/translate.c
> index e5a3f49..4f3cf32 100644
> --- a/target-xtensa/translate.c
> +++ b/target-xtensa/translate.c
> @@ -56,8 +56,6 @@ typedef struct DisasContext {
>  
>      bool sar_5bit;
>      bool sar_m32_5bit;
> -    bool sar_m32_allocated;
> -    TCGv_i32 sar_m32;
>  
>      uint32_t ccount_delta;
>      unsigned used_window;
> @@ -71,6 +69,7 @@ typedef struct DisasContext {
>  
>  static TCGv_ptr cpu_env;
>  static TCGv_i32 cpu_pc;
> +static TCGv_i32 cpu_sar_m32;
>  static TCGv_i32 cpu_R[16];
>  static TCGv_i32 cpu_FR[16];
>  static TCGv_i32 cpu_SR[256];
> @@ -169,6 +168,8 @@ void xtensa_translate_init(void)
>      cpu_env = tcg_global_reg_new_ptr(TCG_AREG0, "env");
>      cpu_pc = tcg_global_mem_new_i32(TCG_AREG0,
>              offsetof(CPUXtensaState, pc), "pc");
> +    cpu_sar_m32 = tcg_global_mem_new_i32(TCG_AREG0,
> +            offsetof(CPUXtensaState, sar_m32), "sar_m32");
>  
>      for (i = 0; i < 16; i++) {
>          cpu_R[i] = tcg_global_mem_new_i32(TCG_AREG0,
> @@ -230,21 +231,13 @@ static void init_sar_tracker(DisasContext *dc)
>  {
>      dc->sar_5bit = false;
>      dc->sar_m32_5bit = false;
> -    dc->sar_m32_allocated = false;
> -}
> -
> -static void reset_sar_tracker(DisasContext *dc)
> -{
> -    if (dc->sar_m32_allocated) {
> -        tcg_temp_free(dc->sar_m32);
> -    }
>  }
>  
>  static void gen_right_shift_sar(DisasContext *dc, TCGv_i32 sa)
>  {
>      tcg_gen_andi_i32(cpu_SR[SAR], sa, 0x1f);
>      if (dc->sar_m32_5bit) {
> -        tcg_gen_discard_i32(dc->sar_m32);
> +        tcg_gen_discard_i32(cpu_sar_m32);
>      }
>      dc->sar_5bit = true;
>      dc->sar_m32_5bit = false;
> @@ -253,12 +246,8 @@ static void gen_right_shift_sar(DisasContext *dc, TCGv_i32 sa)
>  static void gen_left_shift_sar(DisasContext *dc, TCGv_i32 sa)
>  {
>      TCGv_i32 tmp = tcg_const_i32(32);
> -    if (!dc->sar_m32_allocated) {
> -        dc->sar_m32 = tcg_temp_local_new_i32();
> -        dc->sar_m32_allocated = true;
> -    }
> -    tcg_gen_andi_i32(dc->sar_m32, sa, 0x1f);
> -    tcg_gen_sub_i32(cpu_SR[SAR], tmp, dc->sar_m32);
> +    tcg_gen_andi_i32(cpu_sar_m32, sa, 0x1f);
> +    tcg_gen_sub_i32(cpu_SR[SAR], tmp, cpu_sar_m32);
>      dc->sar_5bit = false;
>      dc->sar_m32_5bit = true;
>      tcg_temp_free(tmp);
> @@ -498,7 +487,7 @@ static void gen_wsr_sar(DisasContext *dc, uint32_t sr, TCGv_i32 s)
>  {
>      tcg_gen_andi_i32(cpu_SR[sr], s, 0x3f);
>      if (dc->sar_m32_5bit) {
> -        tcg_gen_discard_i32(dc->sar_m32);
> +        tcg_gen_discard_i32(cpu_sar_m32);
>      }
>      dc->sar_5bit = false;
>      dc->sar_m32_5bit = false;
> @@ -1483,7 +1472,7 @@ static void disas_xtensa_insn(CPUXtensaState *env, DisasContext *dc)
>              case 10: /*SLL*/
>                  gen_window_check2(dc, RRR_R, RRR_S);
>                  if (dc->sar_m32_5bit) {
> -                    tcg_gen_shl_i32(cpu_R[RRR_R], cpu_R[RRR_S], dc->sar_m32);
> +                    tcg_gen_shl_i32(cpu_R[RRR_R], cpu_R[RRR_S], cpu_sar_m32);
>                  } else {
>                      TCGv_i64 v = tcg_temp_new_i64();
>                      TCGv_i32 s = tcg_const_i32(32);
> @@ -2947,7 +2936,6 @@ static void gen_intermediate_code_internal(
>              tcg_ctx.gen_opc_ptr < gen_opc_end);
>  
>      reset_litbase(&dc);
> -    reset_sar_tracker(&dc);
>      if (dc.icount) {
>          tcg_temp_free(dc.next_icount);
>      }
> -- 
> 1.7.7.6
> 
>
Max Filippov - Nov. 24, 2012, 11:27 a.m.
On Sat, Nov 24, 2012 at 2:59 PM, Aurelien Jarno <aurelien@aurel32.net> wrote:
> On Sat, Nov 24, 2012 at 04:51:36AM +0400, Max Filippov wrote:
>> This fixes the following assertion caused by local temp reaching the end
>> of TB in discarded state:
>>
>>   qemu-system-xtensa: tcg/tcg.c:1665: temp_save: Assertion `s->temps[temp].val_type == 2 || s->temps[temp].fixed_reg' failed.
>>   Aborted
>>
>> Signed-off-by: Max Filippov <jcmvbkbc@gmail.com>
>> ---
>>  target-xtensa/cpu.h       |    1 +
>>  target-xtensa/translate.c |   28 ++++++++--------------------
>>  2 files changed, 9 insertions(+), 20 deletions(-)
>
> I have just send a patch to fix the issue in the TCG code instead (sorry
> for being so long, the last weeks have been quite busy). I think it is
> better than fixing the issue in target-xtensa.
>
> If it works for you, I'll commit it.

Works perfectly, thanks. (nothing to be sorry for, BTW).

And it seems to me that switch from local temp to global makes code a bit
cleaner. However, since it's no longer a fix I will hold it until 1.3 is out.

Patch

diff --git a/target-xtensa/cpu.h b/target-xtensa/cpu.h
index 74e9888..f021a9a 100644
--- a/target-xtensa/cpu.h
+++ b/target-xtensa/cpu.h
@@ -323,6 +323,7 @@  typedef struct CPUXtensaState {
     const XtensaConfig *config;
     uint32_t regs[16];
     uint32_t pc;
+    uint32_t sar_m32;
     uint32_t sregs[256];
     uint32_t uregs[256];
     uint32_t phys_regs[MAX_NAREG];
diff --git a/target-xtensa/translate.c b/target-xtensa/translate.c
index e5a3f49..4f3cf32 100644
--- a/target-xtensa/translate.c
+++ b/target-xtensa/translate.c
@@ -56,8 +56,6 @@  typedef struct DisasContext {
 
     bool sar_5bit;
     bool sar_m32_5bit;
-    bool sar_m32_allocated;
-    TCGv_i32 sar_m32;
 
     uint32_t ccount_delta;
     unsigned used_window;
@@ -71,6 +69,7 @@  typedef struct DisasContext {
 
 static TCGv_ptr cpu_env;
 static TCGv_i32 cpu_pc;
+static TCGv_i32 cpu_sar_m32;
 static TCGv_i32 cpu_R[16];
 static TCGv_i32 cpu_FR[16];
 static TCGv_i32 cpu_SR[256];
@@ -169,6 +168,8 @@  void xtensa_translate_init(void)
     cpu_env = tcg_global_reg_new_ptr(TCG_AREG0, "env");
     cpu_pc = tcg_global_mem_new_i32(TCG_AREG0,
             offsetof(CPUXtensaState, pc), "pc");
+    cpu_sar_m32 = tcg_global_mem_new_i32(TCG_AREG0,
+            offsetof(CPUXtensaState, sar_m32), "sar_m32");
 
     for (i = 0; i < 16; i++) {
         cpu_R[i] = tcg_global_mem_new_i32(TCG_AREG0,
@@ -230,21 +231,13 @@  static void init_sar_tracker(DisasContext *dc)
 {
     dc->sar_5bit = false;
     dc->sar_m32_5bit = false;
-    dc->sar_m32_allocated = false;
-}
-
-static void reset_sar_tracker(DisasContext *dc)
-{
-    if (dc->sar_m32_allocated) {
-        tcg_temp_free(dc->sar_m32);
-    }
 }
 
 static void gen_right_shift_sar(DisasContext *dc, TCGv_i32 sa)
 {
     tcg_gen_andi_i32(cpu_SR[SAR], sa, 0x1f);
     if (dc->sar_m32_5bit) {
-        tcg_gen_discard_i32(dc->sar_m32);
+        tcg_gen_discard_i32(cpu_sar_m32);
     }
     dc->sar_5bit = true;
     dc->sar_m32_5bit = false;
@@ -253,12 +246,8 @@  static void gen_right_shift_sar(DisasContext *dc, TCGv_i32 sa)
 static void gen_left_shift_sar(DisasContext *dc, TCGv_i32 sa)
 {
     TCGv_i32 tmp = tcg_const_i32(32);
-    if (!dc->sar_m32_allocated) {
-        dc->sar_m32 = tcg_temp_local_new_i32();
-        dc->sar_m32_allocated = true;
-    }
-    tcg_gen_andi_i32(dc->sar_m32, sa, 0x1f);
-    tcg_gen_sub_i32(cpu_SR[SAR], tmp, dc->sar_m32);
+    tcg_gen_andi_i32(cpu_sar_m32, sa, 0x1f);
+    tcg_gen_sub_i32(cpu_SR[SAR], tmp, cpu_sar_m32);
     dc->sar_5bit = false;
     dc->sar_m32_5bit = true;
     tcg_temp_free(tmp);
@@ -498,7 +487,7 @@  static void gen_wsr_sar(DisasContext *dc, uint32_t sr, TCGv_i32 s)
 {
     tcg_gen_andi_i32(cpu_SR[sr], s, 0x3f);
     if (dc->sar_m32_5bit) {
-        tcg_gen_discard_i32(dc->sar_m32);
+        tcg_gen_discard_i32(cpu_sar_m32);
     }
     dc->sar_5bit = false;
     dc->sar_m32_5bit = false;
@@ -1483,7 +1472,7 @@  static void disas_xtensa_insn(CPUXtensaState *env, DisasContext *dc)
             case 10: /*SLL*/
                 gen_window_check2(dc, RRR_R, RRR_S);
                 if (dc->sar_m32_5bit) {
-                    tcg_gen_shl_i32(cpu_R[RRR_R], cpu_R[RRR_S], dc->sar_m32);
+                    tcg_gen_shl_i32(cpu_R[RRR_R], cpu_R[RRR_S], cpu_sar_m32);
                 } else {
                     TCGv_i64 v = tcg_temp_new_i64();
                     TCGv_i32 s = tcg_const_i32(32);
@@ -2947,7 +2936,6 @@  static void gen_intermediate_code_internal(
             tcg_ctx.gen_opc_ptr < gen_opc_end);
 
     reset_litbase(&dc);
-    reset_sar_tracker(&dc);
     if (dc.icount) {
         tcg_temp_free(dc.next_icount);
     }