* [Qemu-devel] [PATCH] Fix extlh instruction on Alpha @ 2009-09-09 16:08 Vince Weaver 2009-09-16 19:52 ` Aurelien Jarno 0 siblings, 1 reply; 9+ messages in thread From: Vince Weaver @ 2009-09-09 16:08 UTC (permalink / raw) To: qemu-devel (re-sending) The extlh instruction on Alpha currently doesn't work properly. It's a combination of a cut/paste bug (16 where it should be 32) as well as a "shift by 64" bug. Below is a patch that fixes the problem. The previous e-mails on this problem have test cases that exhibit the bug. This patch uses tcg_temp_local_new() at the suggestion of Filip Navara. Vince Signed-off-by: Vince Weaver <vince@csl.cornell.edu> diff --git a/target-alpha/translate.c b/target-alpha/translate.c index 1fc5119..4219916 100644 --- a/target-alpha/translate.c +++ b/target-alpha/translate.c @@ -526,14 +526,24 @@ static always_inline void gen_ext_h(void (*tcg_gen_ext_i64)(TCGv t0, TCGv t1), else tcg_gen_mov_i64(cpu_ir[rc], cpu_ir[ra]); } else { + int l1; TCGv tmp1, tmp2; - tmp1 = tcg_temp_new(); + tmp1 = tcg_temp_local_new(); + l1 = gen_new_label(); + tcg_gen_andi_i64(tmp1, cpu_ir[rb], 7); tcg_gen_shli_i64(tmp1, tmp1, 3); + + tcg_gen_mov_i64(cpu_ir[rc], cpu_ir[ra]); + tcg_gen_brcondi_i64(TCG_COND_EQ, tmp1, 0, l1); + tmp2 = tcg_const_i64(64); tcg_gen_sub_i64(tmp1, tmp2, tmp1); tcg_temp_free(tmp2); tcg_gen_shl_i64(cpu_ir[rc], cpu_ir[ra], tmp1); + + gen_set_label(l1); + tcg_temp_free(tmp1); } if (tcg_gen_ext_i64) @@ -1320,7 +1330,7 @@ static always_inline int translate_one (DisasContext *ctx, uint32_t insn) break; case 0x6A: /* EXTLH */ - gen_ext_h(&tcg_gen_ext16u_i64, ra, rb, rc, islit, lit); + gen_ext_h(&tcg_gen_ext32u_i64, ra, rb, rc, islit, lit); break; case 0x72: /* MSKQH */ ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [PATCH] Fix extlh instruction on Alpha 2009-09-09 16:08 [Qemu-devel] [PATCH] Fix extlh instruction on Alpha Vince Weaver @ 2009-09-16 19:52 ` Aurelien Jarno 2009-09-16 20:45 ` Vince Weaver 0 siblings, 1 reply; 9+ messages in thread From: Aurelien Jarno @ 2009-09-16 19:52 UTC (permalink / raw) To: Vince Weaver; +Cc: qemu-devel On Wed, Sep 09, 2009 at 12:08:25PM -0400, Vince Weaver wrote: > > (re-sending) > > The extlh instruction on Alpha currently doesn't work properly. > It's a combination of a cut/paste bug (16 where it should be 32) as well > as a "shift by 64" bug. > > Below is a patch that fixes the problem. The previous e-mails on this > problem have test cases that exhibit the bug. > > This patch uses tcg_temp_local_new() at the suggestion of Filip Navara. > > Vince > > Signed-off-by: Vince Weaver <vince@csl.cornell.edu> > > diff --git a/target-alpha/translate.c b/target-alpha/translate.c > index 1fc5119..4219916 100644 > --- a/target-alpha/translate.c > +++ b/target-alpha/translate.c > @@ -526,14 +526,24 @@ static always_inline void gen_ext_h(void (*tcg_gen_ext_i64)(TCGv t0, TCGv t1), > else > tcg_gen_mov_i64(cpu_ir[rc], cpu_ir[ra]); > } else { > + int l1; > TCGv tmp1, tmp2; > - tmp1 = tcg_temp_new(); > + tmp1 = tcg_temp_local_new(); > + l1 = gen_new_label(); > + > tcg_gen_andi_i64(tmp1, cpu_ir[rb], 7); > tcg_gen_shli_i64(tmp1, tmp1, 3); > + > + tcg_gen_mov_i64(cpu_ir[rc], cpu_ir[ra]); > + tcg_gen_brcondi_i64(TCG_COND_EQ, tmp1, 0, l1); > + > tmp2 = tcg_const_i64(64); > tcg_gen_sub_i64(tmp1, tmp2, tmp1); > tcg_temp_free(tmp2); Given that a test costs a lot (partly due to the fact temp local variable must be used), I do wonder if doing a AND here wouldn't be better: tcg_gen_andi_i64(tmp1, tmp1, 0x3f); > tcg_gen_shl_i64(cpu_ir[rc], cpu_ir[ra], tmp1); > + > + gen_set_label(l1); > + > tcg_temp_free(tmp1); > } > if (tcg_gen_ext_i64) > @@ -1320,7 +1330,7 @@ static always_inline int translate_one (DisasContext *ctx, uint32_t insn) > break; > case 0x6A: > /* EXTLH */ > - gen_ext_h(&tcg_gen_ext16u_i64, ra, rb, rc, islit, lit); > + gen_ext_h(&tcg_gen_ext32u_i64, ra, rb, rc, islit, lit); > break; > case 0x72: > /* MSKQH */ > > > -- Aurelien Jarno GPG: 1024D/F1BCDB73 aurelien@aurel32.net http://www.aurel32.net ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [PATCH] Fix extlh instruction on Alpha 2009-09-16 19:52 ` Aurelien Jarno @ 2009-09-16 20:45 ` Vince Weaver 2009-09-16 20:56 ` Aurelien Jarno 2009-09-16 21:14 ` [Qemu-devel] " Andreas Schwab 0 siblings, 2 replies; 9+ messages in thread From: Vince Weaver @ 2009-09-16 20:45 UTC (permalink / raw) To: Aurelien Jarno; +Cc: qemu-devel On Wed, 16 Sep 2009, Aurelien Jarno wrote: > On Wed, Sep 09, 2009 at 12:08:25PM -0400, Vince Weaver wrote: > > } else { > > + int l1; > > TCGv tmp1, tmp2; > > - tmp1 = tcg_temp_new(); > > + tmp1 = tcg_temp_local_new(); > > + l1 = gen_new_label(); > > + > > tcg_gen_andi_i64(tmp1, cpu_ir[rb], 7); > > tcg_gen_shli_i64(tmp1, tmp1, 3); > > + > > + tcg_gen_mov_i64(cpu_ir[rc], cpu_ir[ra]); > > + tcg_gen_brcondi_i64(TCG_COND_EQ, tmp1, 0, l1); > > + > > tmp2 = tcg_const_i64(64); > > tcg_gen_sub_i64(tmp1, tmp2, tmp1); > > tcg_temp_free(tmp2); > > Given that a test costs a lot (partly due to the fact temp local > variable must be used), I do wonder if doing a AND here wouldn't > be better: > > tcg_gen_andi_i64(tmp1, tmp1, 0x3f); I'm not sure I follow. The code is attempting the following: tmp1=rb&0x7; tmp1=temp1<<3; if (tmp1!=0) { tmp1=64-tmp1; rc=ra<<tmp1; } else { rc=ra; } The problem with the original code is that in the case of tmp1 being 0, the shift left by 64 would result in 0, instead of the identity. I tried to avoid the jump but couldn't. Am I missing something? > > tcg_gen_shl_i64(cpu_ir[rc], cpu_ir[ra], tmp1); > > + > > + gen_set_label(l1); > > + Vince ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [PATCH] Fix extlh instruction on Alpha 2009-09-16 20:45 ` Vince Weaver @ 2009-09-16 20:56 ` Aurelien Jarno 2009-09-17 16:07 ` Vince Weaver 2009-09-16 21:14 ` [Qemu-devel] " Andreas Schwab 1 sibling, 1 reply; 9+ messages in thread From: Aurelien Jarno @ 2009-09-16 20:56 UTC (permalink / raw) To: Vince Weaver; +Cc: qemu-devel On Wed, Sep 16, 2009 at 04:45:20PM -0400, Vince Weaver wrote: > On Wed, 16 Sep 2009, Aurelien Jarno wrote: > > > On Wed, Sep 09, 2009 at 12:08:25PM -0400, Vince Weaver wrote: > > > > } else { > > > + int l1; > > > TCGv tmp1, tmp2; > > > - tmp1 = tcg_temp_new(); > > > + tmp1 = tcg_temp_local_new(); > > > + l1 = gen_new_label(); > > > + > > > tcg_gen_andi_i64(tmp1, cpu_ir[rb], 7); > > > tcg_gen_shli_i64(tmp1, tmp1, 3); > > > + > > > + tcg_gen_mov_i64(cpu_ir[rc], cpu_ir[ra]); > > > + tcg_gen_brcondi_i64(TCG_COND_EQ, tmp1, 0, l1); > > > + > > > tmp2 = tcg_const_i64(64); > > > tcg_gen_sub_i64(tmp1, tmp2, tmp1); > > > tcg_temp_free(tmp2); > > > > Given that a test costs a lot (partly due to the fact temp local > > variable must be used), I do wonder if doing a AND here wouldn't > > be better: > > > > tcg_gen_andi_i64(tmp1, tmp1, 0x3f); > > I'm not sure I follow. > > The code is attempting the following: > > tmp1=rb&0x7; > tmp1=temp1<<3; > > if (tmp1!=0) { > tmp1=64-tmp1; > rc=ra<<tmp1; > } > else { > rc=ra; > } > > The problem with the original code is that in the case of tmp1 being 0, > the shift left by 64 would result in 0, instead of the identity. > > I tried to avoid the jump but couldn't. Am I missing something? > I mean the following code: tmp1=rb&0x7; tmp1=temp1<<3; tmp1=64-tmp1; tmp1=tmp1 & 0x3f; rc=ra<<tmp1; In case tmp1 = 0, it becomes 64, and then 0 again after the and, so rc=ra<<0. -- Aurelien Jarno GPG: 1024D/F1BCDB73 aurelien@aurel32.net http://www.aurel32.net ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [PATCH] Fix extlh instruction on Alpha 2009-09-16 20:56 ` Aurelien Jarno @ 2009-09-17 16:07 ` Vince Weaver 2009-09-17 16:25 ` Laurent Desnogues ` (2 more replies) 0 siblings, 3 replies; 9+ messages in thread From: Vince Weaver @ 2009-09-17 16:07 UTC (permalink / raw) To: Aurelien Jarno; +Cc: qemu-devel On Wed, 16 Sep 2009, Aurelien Jarno wrote: > In case tmp1 = 0, it becomes 64, and then 0 again after the and, so > rc=ra<<0. Ah, I see. I completely missed that optimization. How does this updated patch look? I removed one of the TCGv variables too. Does that help performance? What would be nice is a tcg subtract-from instruction, which I know some architectures have. Maybe tcg does have it and I should look harder. diff --git a/target-alpha/translate.c b/target-alpha/translate.c index 9d2bc45..af2a43c 100644 --- a/target-alpha/translate.c +++ b/target-alpha/translate.c @@ -524,14 +524,16 @@ static inline void gen_ext_h(void(*tcg_gen_ext_i64)(TCGv t0, TCGv t1), else tcg_gen_mov_i64(cpu_ir[rc], cpu_ir[ra]); } else { - TCGv tmp1, tmp2; + TCGv tmp1; tmp1 = tcg_temp_new(); + tcg_gen_andi_i64(tmp1, cpu_ir[rb], 7); tcg_gen_shli_i64(tmp1, tmp1, 3); - tmp2 = tcg_const_i64(64); - tcg_gen_sub_i64(tmp1, tmp2, tmp1); - tcg_temp_free(tmp2); + tcg_gen_andi_i64(tmp1, tmp1, 0x3f); + tcg_gen_neg_i64(tmp1, tmp1); + tcg_gen_addi_i64(tmp1, tmp1, 64); tcg_gen_shl_i64(cpu_ir[rc], cpu_ir[ra], tmp1); + tcg_temp_free(tmp1); } if (tcg_gen_ext_i64) @@ -1316,7 +1318,7 @@ static inline int translate_one(DisasContext *ctx, uint32_t insn) break; case 0x6A: /* EXTLH */ - gen_ext_h(&tcg_gen_ext16u_i64, ra, rb, rc, islit, lit); + gen_ext_h(&tcg_gen_ext32u_i64, ra, rb, rc, islit, lit); break; case 0x72: /* MSKQH */ ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [PATCH] Fix extlh instruction on Alpha 2009-09-17 16:07 ` Vince Weaver @ 2009-09-17 16:25 ` Laurent Desnogues 2009-09-17 16:35 ` [Qemu-devel] " Andreas Schwab 2009-09-17 17:19 ` [Qemu-devel] " Aurelien Jarno 2 siblings, 0 replies; 9+ messages in thread From: Laurent Desnogues @ 2009-09-17 16:25 UTC (permalink / raw) To: Vince Weaver; +Cc: qemu-devel, Aurelien Jarno On Thu, Sep 17, 2009 at 6:07 PM, Vince Weaver <vince@csl.cornell.edu> wrote: > > What would be nice is a tcg > subtract-from instruction, which I know some architectures have. Maybe > tcg does have it and I should look harder. There is tcg_gen_subfi. Laurent ^ permalink raw reply [flat|nested] 9+ messages in thread
* [Qemu-devel] Re: [PATCH] Fix extlh instruction on Alpha 2009-09-17 16:07 ` Vince Weaver 2009-09-17 16:25 ` Laurent Desnogues @ 2009-09-17 16:35 ` Andreas Schwab 2009-09-17 17:19 ` [Qemu-devel] " Aurelien Jarno 2 siblings, 0 replies; 9+ messages in thread From: Andreas Schwab @ 2009-09-17 16:35 UTC (permalink / raw) To: Vince Weaver; +Cc: qemu-devel, Aurelien Jarno Vince Weaver <vince@csl.cornell.edu> writes: > tcg_gen_andi_i64(tmp1, cpu_ir[rb], 7); > tcg_gen_shli_i64(tmp1, tmp1, 3); > - tmp2 = tcg_const_i64(64); > - tcg_gen_sub_i64(tmp1, tmp2, tmp1); > - tcg_temp_free(tmp2); > + tcg_gen_andi_i64(tmp1, tmp1, 0x3f); > + tcg_gen_neg_i64(tmp1, tmp1); > + tcg_gen_addi_i64(tmp1, tmp1, 64); This wastes an operation. If you switch andi and neg you don't need to add 64. Andreas. -- Andreas Schwab, schwab@linux-m68k.org GPG Key fingerprint = 58CA 54C7 6D53 942B 1756 01D3 44D5 214B 8276 4ED5 "And now for something completely different." ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [PATCH] Fix extlh instruction on Alpha 2009-09-17 16:07 ` Vince Weaver 2009-09-17 16:25 ` Laurent Desnogues 2009-09-17 16:35 ` [Qemu-devel] " Andreas Schwab @ 2009-09-17 17:19 ` Aurelien Jarno 2 siblings, 0 replies; 9+ messages in thread From: Aurelien Jarno @ 2009-09-17 17:19 UTC (permalink / raw) To: Vince Weaver; +Cc: qemu-devel On Thu, Sep 17, 2009 at 12:07:23PM -0400, Vince Weaver wrote: > On Wed, 16 Sep 2009, Aurelien Jarno wrote: > > > In case tmp1 = 0, it becomes 64, and then 0 again after the and, so > > rc=ra<<0. > > Ah, I see. I completely missed that optimization. > > How does this updated patch look? I removed one of the TCGv variables > too. Does that help performance? What would be nice is a tcg Yes it looks ok. Removing one normal TCGv variable doesn't really help. What helps is removing a tcg temp_local variable or a branch. > subtract-from instruction, which I know some architectures have. Maybe > tcg does have it and I should look harder. You can use tcg_gen_subfi_i64 (tcg result, immediate, tcg arg). > diff --git a/target-alpha/translate.c b/target-alpha/translate.c > index 9d2bc45..af2a43c 100644 > --- a/target-alpha/translate.c > +++ b/target-alpha/translate.c > @@ -524,14 +524,16 @@ static inline void gen_ext_h(void(*tcg_gen_ext_i64)(TCGv t0, TCGv t1), > else > tcg_gen_mov_i64(cpu_ir[rc], cpu_ir[ra]); > } else { > - TCGv tmp1, tmp2; ap> + TCGv tmp1; > tmp1 = tcg_temp_new(); > + > tcg_gen_andi_i64(tmp1, cpu_ir[rb], 7); > tcg_gen_shli_i64(tmp1, tmp1, 3); > - tmp2 = tcg_const_i64(64); > - tcg_gen_sub_i64(tmp1, tmp2, tmp1); > - tcg_temp_free(tmp2); > + tcg_gen_andi_i64(tmp1, tmp1, 0x3f); > + tcg_gen_neg_i64(tmp1, tmp1); > + tcg_gen_addi_i64(tmp1, tmp1, 64); > tcg_gen_shl_i64(cpu_ir[rc], cpu_ir[ra], tmp1); > + > tcg_temp_free(tmp1); > } > if (tcg_gen_ext_i64) > @@ -1316,7 +1318,7 @@ static inline int translate_one(DisasContext *ctx, uint32_t insn) > break; > case 0x6A: > /* EXTLH */ > - gen_ext_h(&tcg_gen_ext16u_i64, ra, rb, rc, islit, lit); > + gen_ext_h(&tcg_gen_ext32u_i64, ra, rb, rc, islit, lit); > break; > case 0x72: > /* MSKQH */ > -- Aurelien Jarno GPG: 1024D/F1BCDB73 aurelien@aurel32.net http://www.aurel32.net ^ permalink raw reply [flat|nested] 9+ messages in thread
* [Qemu-devel] Re: [PATCH] Fix extlh instruction on Alpha 2009-09-16 20:45 ` Vince Weaver 2009-09-16 20:56 ` Aurelien Jarno @ 2009-09-16 21:14 ` Andreas Schwab 1 sibling, 0 replies; 9+ messages in thread From: Andreas Schwab @ 2009-09-16 21:14 UTC (permalink / raw) To: Vince Weaver; +Cc: qemu-devel, Aurelien Jarno Vince Weaver <vince@csl.cornell.edu> writes: > The code is attempting the following: > > tmp1=rb&0x7; > tmp1=temp1<<3; > > if (tmp1!=0) { > tmp1=64-tmp1; > rc=ra<<tmp1; > } > else { > rc=ra; > } > > The problem with the original code is that in the case of tmp1 being 0, > the shift left by 64 would result in 0, instead of the identity. > > I tried to avoid the jump but couldn't. Am I missing something? Instead of tmp1 = 64 - tmp1 use tmp1 = -tmp1 & 0x3f. Andreas. -- Andreas Schwab, schwab@linux-m68k.org GPG Key fingerprint = 58CA 54C7 6D53 942B 1756 01D3 44D5 214B 8276 4ED5 "And now for something completely different." ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2009-09-17 17:19 UTC | newest] Thread overview: 9+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2009-09-09 16:08 [Qemu-devel] [PATCH] Fix extlh instruction on Alpha Vince Weaver 2009-09-16 19:52 ` Aurelien Jarno 2009-09-16 20:45 ` Vince Weaver 2009-09-16 20:56 ` Aurelien Jarno 2009-09-17 16:07 ` Vince Weaver 2009-09-17 16:25 ` Laurent Desnogues 2009-09-17 16:35 ` [Qemu-devel] " Andreas Schwab 2009-09-17 17:19 ` [Qemu-devel] " Aurelien Jarno 2009-09-16 21:14 ` [Qemu-devel] " Andreas Schwab
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.