Re: Re: Re: Re: Re: [PATCH V2 1/3] riscv: Add Zicbop instruction definitions & cpufeature

[Andrew Jones]

On Thu, Jan 04, 2024 at 02:43:04PM -0300, Leonardo Bras wrote:
...
> > > > > I don't think we can detect a caller with non-zero offset at compile time, 
> > > > > since it will be used in locks which can be at (potentially) any place in 
> > > > > the block size. (if you have any idea though, please let me know :) )
> > 
> > I forgot to reply to this before. The reason I think it may be possible to
> > validate offset at compile time is because it must be a constant, i.e.
> > __builtin_constant_p(offset) must return true. So maybe something like
> > 
> >  static_assert(__builtin_constant_p(offset) && !(offset & 0x1f))
> > 
> > I'll try to find time to play with it.
> > 
> 
> Let me know if you find anything.

There's nothing we can do in this file (insn-def.h), other than maybe
masking, since all magic must happen at preprocessor time, other than
a tiny bit of constant arithmetic allowed at assembly time. For C, using
a wrapper, like patch 2 of this series introduces, we could add the
static assert above. I'll suggest that in patch 2, since I've already
thought it through, but it sort of feels like overkill to me.

> 
> > > > > 
> > > > > On the other hand, we could create a S-Type macro which deliberately 
> > > > > ignores imm[4:0], like  
> > > > > 
> > > > > + INSN_S_TRUNCATE(OPCODE_OP_IMM, FUNC3(6), __RS2(3),               \
> > > > > +                 SIMM12(offset), RS1(base))
> > > > > 
> > > > > Which saves the bits 11:5 of offset  into imm[11:5], and zero-fill 
> > > > > imm[4:0], like
> > > > > 
> > > > > +#define DEFINE_INSN_S                                                    \
> > > > > + __DEFINE_ASM_GPR_NUMS                                           \
> > > > > +"        .macro insn_s, opcode, func3, rs2, simm12, rs1\n"               \
> > > > > +"        .4byte  ((\\opcode << " __stringify(INSN_S_OPCODE_SHIFT) ") |"  \
> > > > > +"                 (\\func3 << " __stringify(INSN_S_FUNC3_SHIFT) ") |"    \
> > > > > +"                 (.L__gpr_num_\\rs2 << " __stringify(INSN_S_RS2_SHIFT) ") |" \
> > > > > +"                 (.L__gpr_num_\\rs1 << " __stringify(INSN_S_RS1_SHIFT) ") |" \
> > > > > +"                 (((\\simm12 >> 5) & 0x7f) << " __stringify(INSN_S_SIMM7_SHIFT) "))\n" \
> > > > > +"        .endm\n"
> > > > > +
> > > > > 
> > > > > Does this make sense?
> > > > 
> > > > If we create a special version of INSN_S, then I suggest we create one
> > > > where its two SIMM fields are independent and then define prefetch
> > > > instructions like this
> > > > 
> > > >  #define PREFETCH_W(base, offset) \
> > > >      INSN_S_SPLIT_IMM(OPCODE_OP_IMM, FUNC3(6), __RS2(3), \
> > > >          SIMM_11_5(offset >> 5), SIMM_4_0(0), RS1(base))
> > > > 
> > > > which would allow simple review against the spec and potentially
> > > > support other instructions which use hard coded values in the
> > > > immediate fields.
> > > > 
> > > 
> > > I agree, it looks better this way.
> > > 
> > > We could have:
> > > INSN_S_SPLIT_IMM(OPCODE, FUNC3, RS1, RS2, SIMM_11_5, SIMM_4_0)
> > > 
> > > and implement INSN_S like:
> > > #define INSN_S(OPCODE, FUNC3, RS1, RS2, SIMM_11_0) \
> > > 	INSN_S_SPLIT_IMM(OPCODE, FUNC3, RS1, RS2,  \
> > > 		SIMM_11_0 >> 5, SIMM_11_0 & 0x1f)
> > 
> > That won't work since SIMM_11_0 will be a string. Actually, with
> > stringification in mind, I don't think defining INSN_S_SPLIT_IMM()
> > is a good idea.
> 
> I don't see how SIMM_11_0 will be a string here. Is this due to using it 
> on asm code?
> 
> I understand a user will call 
> ---
> PREFETCH_W(base, offset), which becomes:
> 
> INSN_S(OPCODE_OP_IMM, 6, base, 3, offset) , which becomes:
> 
> INSN_S_SPLIT_IMM(OPCODE_OP_IMM, FUNC3(6), RS1(base), RS2(3), \
> 	SIMM_11_5(offset >> 5), SIMM_4_0(offset & 0x1f))

The other annotations, like SIMM12, stringify their arguments. So, if
SIMM_11_5 and SIMM_4_0 also stringified, then it wouldn't be possible
to recombine them into a simm12 for the '.insn s' directive. I suppose
SIMM_11_5 and SIMM_4_0 could just expand their arguments without
stringifying. With that, along with throwing even more ugly at it, then
it is possible to get the end result we want, which is

 - PREFETCH_* instructions are defined with annotations and have a
   SIMM_4_0(0) in their definitions to explicitly point out that field

 - the INSN_S definition still directly maps to the .insn s directive


I got that to work with this

#define __RV_SIMM(v)           v
#define RV___SIMM_11_5(v)      __RV_SIMM(v)
#define RV___SIMM_4_0(v)       __RV_SIMM(v)

#define __INSN_S_SPLIT_IMM(opcode, func3, rs2, simm12, rs1) \
        INSN_S(opcode, func3, rs2, SIMM12(simm12), rs1)

#define INSN_S_SPLIT_IMM(opcode, func3, rs2, simm_11_5, simm_4_0, rs1) \
        __INSN_S_SPLIT_IMM(opcode, func3, rs2, (RV_##simm_11_5 << 5) | RV_##simm_4_0, rs1)

#define CBO_PREFETCH_W(base, offset)                            \
        INSN_S_SPLIT_IMM(OPCODE_OP_IMM, FUNC3(6), __RS2(3),     \
                __SIMM_11_5((offset) >> 5), __SIMM_4_0(0), RS1(base))


But, again, I feel like it's probably overkill...

Thanks,
drew