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

[Leonardo Bras <leobras@redhat.com>]

On Thu, Jan 04, 2024 at 10:47:34AM +0100, Andrew Jones wrote:
> 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@kernel.org 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.

Ok then,

> 
> 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
>

That's right, so it matches ISA documentation :)

 
> > > > >  
> > > > > +#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.
> 

Sorry, I was unable to understand the reasoning.

If we are going to review the numbers sooner or later, would not it be 
better to have the instruction define to have "PREFETCH_W" instead of a 
number, and a unified list of defines for instructions.

This way we don't need to look into the code for 0's 1's and 3's, but 
instead just replace the number in the define list.

What am I missing?  

> > 
> > > 
> > > > +
> > > > + #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.
> 

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)

This would avoid extra instructions in asm while not having duplicated 
code.


> 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.
> 

It makes sense, but I think having INSN_S being implemented with 
INSN_S_SPLIT_IMM like suggested above would allow us to have the 
benefits of a split version without the code duplication.

> Thanks,
> drew
> 

Thanks!
Leo