Re: [Qemu-devel] [PATCH 3/5] tricore: fix RRPW_INSERT instruction

[Brenken, David (EFS-GH2)]

Hi Bastian,

> Hi David,
> 
> On 6/5/19 8:11 AM, David Brenken wrote:
> > From: David Brenken <address@hidden>
> >
> > Signed-off-by: Andreas Konopik <address@hidden>
> > Signed-off-by: David Brenken <address@hidden>
> > Signed-off-by: Georg Hofstetter <address@hidden>
> > Signed-off-by: Robert Rasche <address@hidden>
> > Signed-off-by: Lars Biermanski <address@hidden>
> >
> > ---
> >   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