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

Andrew Jones
2024-01-04T16:41:10Z

On Thu, Jan 04, 2024 at 12:03:57PM -0300, Leonardo Bras wrote:
...
> > > > > 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?

PREFETCH_W isn't defined as just 3, it's defined as
INSN_S(OPCODE_OP_IMM, FUNC3(6), __RS2(3), ...)

Adding a define (PREFETCH_W_RS2?) for the 3 just bloats the code and
requires reviewers of PREFETCH_W to go look up another define.
OPCODE_OP_IMM gets a define because it's used in multiple instructions,
but everything else in an instruction definition should be a number
exactly matching the spec, making it easy to review, or be an argument
passed into the instruction macro.

>
> > >
> > > >
> > > > > +
> > > > > + #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 :) )

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.

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

Thanks,
drew