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

[Andrew Jones]

On Wed, Jan 03, 2024 at 05:33:41PM -0300, Leonardo Bras wrote:
> On Wed, Jan 03, 2024 at 08:29:39PM +0100, Andrew Jones wrote:
> > On Wed, Jan 03, 2024 at 03:52:00PM -0300, Leonardo Bras wrote:
> > > On Sun, Dec 31, 2023 at 03:29:51AM -0500, guoren@xxxxxxxxxx wrote:
...
> > > The shifts seem correct for S-Type, but I would name the IMM defines in a 
> > > way we could understand where they fit in IMM:
> > > 
> > > 
> > > INSN_S_SIMM5_SHIFT -> INSN_S_SIMM_0_4_SHIFT
> > > INSN_S_SIMM7_SHIFT -> INSN_S_SIMM_5_11_SHIFT
> > > 
> > > What do you think?
> > 
> > I'm in favor of this suggestion, but then wonder if we don't need another
> > patch before this which renames INSN_I_SIMM12_SHIFT to
> > INSN_I_SIMM_0_11_SHIFT in order to keep things consistent.
> 
> Agree. If it's ok, I can provide a patch doing the rename on top of this 
> patchset.

The INSN_I change is only needed if we also take the new INSN_S shift
macros, so I think the INSN_I change should be part of this series.

BTW, I just noticed we wrote the numbers backwards. They should be

 INSN_I_SIMM_11_0_SHIFT
 INSN_S_SIMM_11_5_SHIFT
 INSN_S_SIMM_4_0_SHIFT

> > > >  
> > > > +#define CBO_PREFETCH_I(base, offset)				\
> > > > +	INSN_S(OPCODE_OP_IMM, FUNC3(6), __RS2(0),		\
> > > > +	       SIMM12(offset), RS1(base))
> > > > +
> > > > +#define CBO_PREFETCH_R(base, offset)				\
> > > > +	INSN_S(OPCODE_OP_IMM, FUNC3(6), __RS2(1),		\
> > > > +	       SIMM12(offset), RS1(base))
> > > > +
> > > > +#define CBO_PREFETCH_W(base, offset)				\
> > > > +	INSN_S(OPCODE_OP_IMM, FUNC3(6), __RS2(3),		\
> > > > +	       SIMM12(offset), RS1(base))
> > > > +
> > > 
> > > For OP_IMM & FUNC3(6) we have ORI, right?
> > > For ORI, rd will be at bytes 11:7, which in PREFETCH.{i,r,w} is
> > > offset[4:0].
> > > 
> > > IIUC, when the cpu does not support ZICBOP, this should be fine as long as 
> > > rd = 0, since changes to r0 are disregarded.
> > > 
> > > In this case, we need to guarantee offset[4:0] = 0, or else we migth write 
> > > on an unrelated register. This can be noticed in ZICBOP documentation pages 
> > > 21, 22, 23, as offset[4:0] is always [0 0 0 0 0]. 
> > > (Google docs in first comment)
> > > 
> > > What we need here is something like:
> > > + enum {
> > > + 	PREFETCH_I,
> > > + 	PREFETCH_R,
> > > + 	PREFETCH_W,
> > > + }	 
> > 
> > Can't use enum. This header may be included in assembly.
> 
> Oh, I suggest defines then, since it's better to make it clear instead of 
> using 0, 1, 3.

I don't think we gain anything by adding another define in order to create
the instruction define. We have to review the number sooner or later. I'd
prefer we use the number inside the instruction define so we only need
to look one place, which is also consistent with how we use FUNC fields.

> 
> > 
> > > +
> > > + #define CBO_PREFETCH(type, base, offset)                      \
> > > +     INSN_S(OPCODE_OP_IMM, FUNC3(6), __RS2(type),              \
> > > +            SIMM12(offset & ~0x1f), RS1(base))
> > 
> > Yes. I suggested we mask offset as well, but ideally we'd detect a caller
> > using an offset with nonzero lower 5 bits at compile time.
> 
> I would suggest the compiler would take care of this, but I am not sure 
> about the assembly, since I am not sure if it gets any optimization.
> 
> 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 :) )
> 
> 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.

But I'm not sure it's worth it. I think

 #define PREFETCH_W(base, offset) \
     INSN_S(OPCODE_OP_IMM, FUNC3(6), __RS2(3), \
         SIMM12(offset & ~0x1f), RS1(base))

is also pretty easy to review against the spec and we don't have any other
instructions yet with other requirements for the immediates.

Thanks,
drew