Re: [Qemu-devel] [PATCH 16/28] target/riscv: Convert quadrant 1 of RVXC insns to decodetree

[Palmer Dabbelt]

On Fri, 19 Oct 2018 08:28:38 PDT (-0700), address@hidden wrote:
> On 10/13/18 8:53 PM, Richard Henderson wrote:
> > On 10/12/18 10:30 AM, Bastian Koppelmann wrote:
> > > +static bool trans_c_addi(DisasContext *ctx, arg_c_addi *a, uint16_t insn)
> > > +{
> > > +    if (a->imm == 0) {
> > > +        return true;
> > > +    }
> > return false, I think.
>
>
> Those are HINTS, which means the instruction in valid, but does not
> affect state, so true is correct. If I do return false, then Linux does
> not boot anymore :)

Technically, "c.addi x0, 0" is an illegal instruction.  It just so happens, 
however, that the encoding that would arise from "c.addi x0, 0" is instead the 
legal "c.nop" instruction, which happens to have exactly the same effect as a 
"c.addi x0, 0".  No idea why the spec is written this way.

So I guess you're both correct: "trans_c_addi" should treat this as invalid, as 
it's not an addi.  The processor's behavior will still be correct with this 
implementation, though, so I don't see this as a distinction worth worrying 
about.

> > > +    arg_jal arg = { .rd = 1, .imm = a->imm };
> > > +    return trans_jal(ctx, &arg, insn);
> > > +#else
> > > +    /* C.ADDIW */
> > > +    arg_addiw arg = { .rd = a->rd, .rs1 = a->rd, .imm = a->imm };
> > > +    return trans_addiw(ctx, &arg, insn);
> > > +#endif
> > > +}
> > > +
> > > +static bool trans_c_li(DisasContext *ctx, arg_c_li *a, uint16_t insn)
> > > +{
> > > +    if (a->rd == 0) {
> > > +        return true;
> > > +    }
> > return false.
>
>
> Likewise.

In this case I believe "li x0, *" should be invalid.  According to v2.2

   C.LI loads the sign-extended 6-bit immediate, imm, into register rd. C.LI 
   is only valid when rd6 = x0.
   C.LI expands into addi rd, x0, imm[5:0].

> > > +static bool trans_c_addi16sp_lui(DisasContext *ctx, arg_c_addi16sp_lui *a,
> > > +        uint16_t insn)
> > > +{
> > > +    if (a->rd == 2) {
> > > +        /* C.ADDI16SP */
> > > +        arg_addi arg = { .rd = 2, .rs1 = 2, .imm = a->imm_addi16sp };
> > > +        return trans_addi(ctx, &arg, insn);
> > > +    } else if (a->imm_lui != 0) {
> > > +        if (a->rd == 0) {
> > > +            return true;
> > > +        }
> > I think it should be
> >
> >    } else if (a->imm_lui != 0 && a->rd != 0) {
>
> Likewise.

Yes, c.lui is invalid with a non-zero immediate.  Again, according to v2.2

   C.LUI loads the non-zero 6-bit immediate field into bits 17–12 of the 
   destination register, clears the bottom 12 bits, and sign-extends bit 17 
   into all higher bits of the destination. C.LUI is only valid when rd6 = 
   {x0, x2}, and when the immediate is not equal to zero.  C.LUI expands into 
   lui rd, nzuimm[17:12].