Re: [Qemu-devel] [PATCH v14 24/33] target-tilegx: Handle shift instructions

[Richard Henderson]

On 08/30/2015 06:38 AM, Peter Maydell wrote:
> On 24 August 2015 at 17:17, Richard Henderson <address@hidden> wrote:
> > Signed-off-by: Richard Henderson <address@hidden>
> > ---
> >   target-tilegx/translate.c | 57 +++++++++++++++++++++++++++++++++++++++++++++--
> >   1 file changed, 55 insertions(+), 2 deletions(-)
> >
> > diff --git a/target-tilegx/translate.c b/target-tilegx/translate.c
> > index 6be751b..4e6d577 100644
> > --- a/target-tilegx/translate.c
> > +++ b/target-tilegx/translate.c
> > @@ -474,6 +474,7 @@ static TileExcp gen_rrr_opcode(DisasContext *dc, unsigned 
> > opext,
> >       TCGv tdest = dest_gr(dc, dest);
> >       TCGv tsrca = load_gr(dc, srca);
> >       TCGv tsrcb = load_gr(dc, srcb);
> > +    TCGv t0;
>
> Personally I would restrict the scope of this to just the
> case block where it's used.

It'll get used lots more by other insns in subsequent patches.  While 
restricting the scope is arguably ideal, it adds 3 lines for each use and 
brevity has its own appeal.

> Using shl for a shift right doesn't look right...
...
> Shift left used when shift right intended ?

Fixed.

> > @@ -1096,6 +1148,7 @@ static TileExcp gen_rri_opcode(DisasContext *dc, unsigned 
> > opext,
> >       case OE_SH(V2SHRSI, X1):
> >       case OE_SH(V2SHRUI, X0):
> >       case OE_SH(V2SHRUI, X1):
> > +        return TILEGX_EXCP_OPCODE_UNIMPLEMENTED;
>
> Why does this change appear in this patch?

I'm sure I meant to rebase it into the patch implementing ADDLI.

> >       case OE(ADDLI_OPCODE_X0, 0, X0):
> >       case OE(ADDLI_OPCODE_X1, 0, X1):


r~