| Message ID | 20190605061126.10244-4-david.brenken@efs-auto.org |
|---|---|
| State | New |
| Headers | show |
| Series | tricore: adding new instructions and fixing issues | expand |
Hi David, On 6/5/19 8:11 AM, David Brenken wrote: > From: David Brenken <david.brenken@efs-auto.de> > > Signed-off-by: Andreas Konopik <andreas.konopik@efs-auto.de> > Signed-off-by: David Brenken <david.brenken@efs-auto.de> > Signed-off-by: Georg Hofstetter <georg.hofstetter@efs-auto.de> > Signed-off-by: Robert Rasche <robert.rasche@efs-auto.de> > Signed-off-by: Lars Biermanski <lars.biermanski@efs-auto.de> > > --- > target/tricore/translate.c | 11 ++++++++--- > 1 file changed, 8 insertions(+), 3 deletions(-) > > diff --git a/target/tricore/translate.c b/target/tricore/translate.c > index a8b4de647a..19d8db7a46 100644 > --- a/target/tricore/translate.c > +++ b/target/tricore/translate.c > @@ -7004,6 +7004,7 @@ static void decode_rrpw_extract_insert(CPUTriCoreState *env, DisasContext *ctx) > uint32_t op2; > int r1, r2, r3; > int32_t pos, width; > + TCGv temp1, temp2; > > op2 = MASK_OP_RRPW_OP2(ctx->opcode); > r1 = MASK_OP_RRPW_S1(ctx->opcode); > @@ -7042,9 +7043,13 @@ static void decode_rrpw_extract_insert(CPUTriCoreState *env, DisasContext *ctx) > } > break; > case OPC2_32_RRPW_INSERT: > - if (pos + width <= 31) { > - tcg_gen_deposit_tl(cpu_gpr_d[r3], cpu_gpr_d[r1], cpu_gpr_d[r2], > - width, pos); Can you explain the problem causing the bug? Deposit looks fine to me. After reading the specs again, I agree that the check needs to be <= 32. > + if (pos + width <= 32) { > + temp1 = tcg_const_i32(pos); /* pos */ > + temp2 = tcg_const_i32(width); /* width */ Useless comments. Cheers, Bastian
Hi Bastian, > Hi David, > > On 6/5/19 8:11 AM, David Brenken wrote: > > From: David Brenken <david.brenken@efs-auto.de> > > > > Signed-off-by: Andreas Konopik <andreas.konopik@efs-auto.de> > > Signed-off-by: David Brenken <david.brenken@efs-auto.de> > > Signed-off-by: Georg Hofstetter <georg.hofstetter@efs-auto.de> > > Signed-off-by: Robert Rasche <robert.rasche@efs-auto.de> > > Signed-off-by: Lars Biermanski <lars.biermanski@efs-auto.de> > > > > --- > > target/tricore/translate.c | 11 ++++++++--- > > 1 file changed, 8 insertions(+), 3 deletions(-) > > > > diff --git a/target/tricore/translate.c b/target/tricore/translate.c > > index a8b4de647a..19d8db7a46 100644 > > --- a/target/tricore/translate.c > > +++ b/target/tricore/translate.c > > @@ -7004,6 +7004,7 @@ static void > decode_rrpw_extract_insert(CPUTriCoreState *env, DisasContext *ctx) > > uint32_t op2; > > int r1, r2, r3; > > int32_t pos, width; > > + TCGv temp1, temp2; > > > > op2 = MASK_OP_RRPW_OP2(ctx->opcode); > > r1 = MASK_OP_RRPW_S1(ctx->opcode); > > @@ -7042,9 +7043,13 @@ static void > decode_rrpw_extract_insert(CPUTriCoreState *env, DisasContext *ctx) > > } > > break; > > case OPC2_32_RRPW_INSERT: > > - if (pos + width <= 31) { > > - tcg_gen_deposit_tl(cpu_gpr_d[r3], cpu_gpr_d[r1], cpu_gpr_d[r2], > > - width, pos); > Can you explain the problem causing the bug? Deposit looks fine to me. > After reading the specs again, I agree that the check needs to be <= 32. The bug was recognized because of different behavior between actual hardware and QEMU. Just from looking at it I would say that deposit masks and then shifts the arg2 (D[b]) while the manual states to first shift D[b] and then mask it. I remember that it was a corner case (e.g. width + pos = 31 or 32). I also thought that using the direct insert instruction is more convenient since it was present anyway. > > > > + if (pos + width <= 32) { > > + temp1 = tcg_const_i32(pos); /* pos */ > > + temp2 = tcg_const_i32(width); /* width */ > Useless comments. I will remove them in a second version of the patchset. > > > Cheers, > > Bastian Best regards David
On 6/6/19 2:26 AM, Brenken, David (EFS-GH2) wrote: >>> case OPC2_32_RRPW_INSERT: >>> - if (pos + width <= 31) { >>> - tcg_gen_deposit_tl(cpu_gpr_d[r3], cpu_gpr_d[r1], cpu_gpr_d[r2], >>> - width, pos); >> Can you explain the problem causing the bug? Deposit looks fine to me. >> After reading the specs again, I agree that the check needs to be <= 32. > The bug was recognized because of different behavior between actual hardware and QEMU. > Just from looking at it I would say that deposit masks and then shifts the arg2 (D[b]) while the > manual states to first shift D[b] and then mask it. I remember that it was a corner case (e.g. > width + pos = 31 or 32). The final two arguments to tcg_gen_deposit_tl are swapped. It should be pos, width. r~
Thank you for your hint. Would anyone mind, if keep my insert implementation anyway? If I compare the pseudo code of the instruction manual to the insert implementation it looks nearly the same whereas it seems kind of difficult to directly map the pseudo code of the instruction manual to the general deposit instruction. Best regards David Brenken
Hi David, On 6/12/19 7:48 AM, Brenken, David (EFS-GH2) wrote: > Thank you for your hint. > Would anyone mind, if keep my insert implementation anyway? > If I compare the pseudo code of the instruction manual to the insert implementation it looks nearly the same whereas it seems kind of difficult to directly map the pseudo code of the instruction manual to the general deposit instruction. I think deposit is better, as tcg can emit the native deposit instruction of the host processor (which it does for x86). gen_insert() on the other hand needs 8 tcg ops. Regarding the pseudo code, take a look at tcg/README. You'll see that the semantics of insert and deposit are the same. Thus, I think Richards fix is better. Cheers, Bastian
diff --git a/target/tricore/translate.c b/target/tricore/translate.c index a8b4de647a..19d8db7a46 100644 --- a/target/tricore/translate.c +++ b/target/tricore/translate.c @@ -7004,6 +7004,7 @@ static void decode_rrpw_extract_insert(CPUTriCoreState *env, DisasContext *ctx) uint32_t op2; int r1, r2, r3; int32_t pos, width; + TCGv temp1, temp2; op2 = MASK_OP_RRPW_OP2(ctx->opcode); r1 = MASK_OP_RRPW_S1(ctx->opcode); @@ -7042,9 +7043,13 @@ static void decode_rrpw_extract_insert(CPUTriCoreState *env, DisasContext *ctx) } break; case OPC2_32_RRPW_INSERT: - if (pos + width <= 31) { - tcg_gen_deposit_tl(cpu_gpr_d[r3], cpu_gpr_d[r1], cpu_gpr_d[r2], - width, pos); + if (pos + width <= 32) { + temp1 = tcg_const_i32(pos); /* pos */ + temp2 = tcg_const_i32(width); /* width */ + gen_insert(cpu_gpr_d[r3], cpu_gpr_d[r1], cpu_gpr_d[r2], + temp1, temp2); + tcg_temp_free(temp1); + tcg_temp_free(temp2); } break; default: