Re: [Qemu-devel] [PATCH v4 5/8] target/mips: R5900 DMULT[U], DDIV[U], LL, SC, LLD and SCD are user only

[Fredrik Noring]

Hi Maciej,

Philippe -- thank you for your reviews,

On Mon, Sep 17, 2018 at 06:10:27PM +0100, Maciej W. Rozycki wrote:
>  Nitpicking here, but I think it's what makes code clean and pleasant to 
> read.

I agree, that is important too. I will post an updated v5 soon. Another
alternative change is to define check_insn_opc_user_only as

static inline void check_insn_opc_user_only(DisasContext *ctx, int flags)
{
#ifndef CONFIG_USER_ONLY
    check_insn_opc_removed(ctx, flags);
#endif
}

by referring to check_insn_opc_removed (instead of copying its definition).
Would you consider this an improvement for v5 too?

>  I think it would make sense to keep the order of the checks consistent, 
> or otherwise code starts looking messy.
> 
>  The predominant order appears to be (as applicable) starting with a 
> check for the lowest ISA membership, followed by a check for the highest 
> ISA membership (R6 instruction cuts) and ending with processor-specific 
> special cases.  I think this order makes sense in that it starts with 
> checks that have a wider scope and then moves on to ones of a narrower 
> scope, and I think keeping it would be good (as would fixing where the 
> addition of R6 broke it).  Mode checks for otherwise existing 
> instructions then follow, which complements the pattern.

Sure, thanks for clarifying the ordering rules!

>  So please make it:
> 
>         check_insn(ctx, ISA_MIPS3);
>         check_insn_opc_user_only(ctx, INSN_R5900);
>         check_mips_64(ctx);

Done.

> > @@ -25275,6 +25280,7 @@ static void decode_opc(CPUMIPSState *env, 
> > DisasContext *ctx)
> >          break;
> >      case OPC_SCD:
> >          check_insn_opc_removed(ctx, ISA_MIPS32R6);
> > +        check_insn_opc_user_only(ctx, INSN_R5900);
> >          check_insn(ctx, ISA_MIPS3);
> >          check_mips_64(ctx);
> >          gen_st_cond(ctx, op, rt, rs, imm);
> 
>  And please make it:
> 
>         check_insn_opc_removed(ctx, ISA_MIPS32R6);
>         check_insn(ctx, ISA_MIPS3);
>         check_insn_opc_user_only(ctx, INSN_R5900);
>         check_mips_64(ctx);

Done.

> here (and swapping the two former calls ought to be fixed separately; I 
> haven't checked if there are more cases like this, but if so, then they 
> would best be amended with a single change).

I'll defer other ordering and indentation fixes since I'm not sure whether
such changes would be accepted.

Fredrik