From: Leonardo Bras <leobras@redhat.com>
To: Andrew Jones <ajones@ventanamicro.com>
Cc: wefu@redhat.com, keescook@chromium.org, atishp@atishpatra.org,
peterz@infradead.org, unicorn_wang@outlook.com,
paul.walmsley@sifive.com, chao.wei@sophgo.com,
bjorn@rivosinc.com, linux-kernel@vger.kernel.org,
xiaoguang.xing@sophgo.com, conor.dooley@microchip.com,
Leonardo Bras <leobras@redhat.com>,
palmer@dabbelt.com, jszhang@kernel.org,
panqinglin2020@iscas.ac.cn, Guo Ren <guoren@linux.alibaba.com>,
guoren@kernel.org, linux-riscv@lists.infradead.org,
wuwei2016@iscas.ac.cn
Subject: Re: Re: Re: Re: [PATCH V2 1/3] riscv: Add Zicbop instruction definitions & cpufeature
Date: Thu, 4 Jan 2024 14:43:04 -0300 [thread overview]
Message-ID: <ZZbuKCvATa7yyQOc@LeoBras> (raw)
In-Reply-To: <20240104-d6981cf63a39af4dff1d380a@orel>
On Thu, Jan 04, 2024 at 05:40:50PM +0100, Andrew Jones wrote:
> 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.
Ok, I see your point now.
It's fine by me, then.
>
> >
> > > >
> > > > >
> > > > > > +
> > > > > > + #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.
>
Let me know if you find anything.
> > > >
> > > > 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))
---
I don't see an issue here, the same wouldwork for every INSN_S()
Now suppose we make PREFETCH_W use SPLIT_IMM directly:
---
PREFETCH_W(base, offset), which becomes:
INSN_S_SPLIT_IMM(OPCODE_OP_IMM, FUNC3(6), RS1(base), RS2(3), \
SIMM_11_5(offset >> 5), SIMM_4_0(0))
---
I don't see how stringification gets in the way.
Thanks!
Leo
#define CBO_PREFETCH(type, base, offset) \
> > > > > > + INSN_S(OPCODE_OP_IMM, FUNC3(6), __RS2(type), \
> > > > > > + SIMM12(offset & ~0x1f), RS1(base))
>
> Thanks,
> drew
>
_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv
next prev parent reply other threads:[~2024-01-04 17:43 UTC|newest]
Thread overview: 42+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-12-31 8:29 [PATCH V2 0/3] riscv: Add Zicbop & prefetchw support guoren
2023-12-31 8:29 ` [PATCH V2 1/3] riscv: Add Zicbop instruction definitions & cpufeature guoren
2024-01-02 10:32 ` Andrew Jones
2024-01-03 6:13 ` Guo Ren
2024-01-03 6:49 ` Andrew Jones
2024-01-03 19:44 ` Andrew Jones
2024-01-03 19:06 ` Leonardo Bras
2024-01-03 9:31 ` Clément Léger
2024-01-03 12:00 ` Andrew Jones
2024-01-11 10:31 ` Clément Léger
2024-01-11 10:45 ` Andrew Jones
2024-01-11 10:49 ` Clément Léger
2024-01-11 11:12 ` Conor Dooley
2024-01-03 18:52 ` Leonardo Bras
2024-01-03 19:29 ` Andrew Jones
2024-01-03 20:33 ` Leonardo Bras
2024-01-04 9:47 ` Andrew Jones
2024-01-04 15:03 ` Leonardo Bras
2024-01-04 16:40 ` Andrew Jones
2024-01-04 17:43 ` Leonardo Bras [this message]
2024-01-05 13:24 ` Andrew Jones
2024-01-08 14:34 ` Leonardo Bras
2024-01-08 15:24 ` Andrew Jones
2024-01-08 16:14 ` Leonardo Bras
2024-01-03 19:48 ` Andrew Jones
2024-01-03 20:34 ` Leonardo Bras
2023-12-31 8:29 ` [PATCH V2 2/3] riscv: Add ARCH_HAS_PRETCHW support with Zibop guoren
2024-01-01 2:29 ` Guo Ren
2024-01-03 19:04 ` Leonardo Bras
2024-01-02 10:45 ` Andrew Jones
2024-01-03 6:19 ` Guo Ren
2024-01-03 19:56 ` Andrew Jones
2024-01-05 13:31 ` Andrew Jones
2023-12-31 8:29 ` [PATCH V2 3/3] riscv: xchg: Prefetch the destination word for sc.w guoren
2024-01-02 11:18 ` Andrew Jones
2024-01-03 6:15 ` Guo Ren
2024-01-03 19:45 ` Leonardo Bras
2024-01-04 1:24 ` Guo Ren
2024-01-04 3:56 ` Leonardo Bras
2024-01-04 8:14 ` Guo Ren
2024-01-04 14:17 ` Leonardo Bras
2024-01-05 1:13 ` Guo Ren
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=ZZbuKCvATa7yyQOc@LeoBras \
--to=leobras@redhat.com \
--cc=ajones@ventanamicro.com \
--cc=atishp@atishpatra.org \
--cc=bjorn@rivosinc.com \
--cc=chao.wei@sophgo.com \
--cc=conor.dooley@microchip.com \
--cc=guoren@kernel.org \
--cc=guoren@linux.alibaba.com \
--cc=jszhang@kernel.org \
--cc=keescook@chromium.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-riscv@lists.infradead.org \
--cc=palmer@dabbelt.com \
--cc=panqinglin2020@iscas.ac.cn \
--cc=paul.walmsley@sifive.com \
--cc=peterz@infradead.org \
--cc=unicorn_wang@outlook.com \
--cc=wefu@redhat.com \
--cc=wuwei2016@iscas.ac.cn \
--cc=xiaoguang.xing@sophgo.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).