Add CMO extension intrinsics #93

Liaoshihua · 2024-10-25T07:19:37Z

Add CMO extension intrinsics description.

topperc · 2024-10-25T23:03:00Z

src/c-api.adoc

+|`+void __riscv_cbo_flush(void *addr);+`                                                  |`cbo.flush`          |Zicbom
+|`+void __riscv_cbo_inval(void *addr);+`                                                  |`cbo.inval`          |Zicbom
+|`+void __riscv_cbo_zero(void *addr);+`                                                   |`cbo.zero`           |Zicboz
+|`+void __riscv_cbo_prefetch(void *addr, const int rw, const int locality);+`             |`prefetch.[r][w]`    |Zicbop


Is __riscv_cbo_prefetch a wrapper around __builtin_prefetch where there are 4 possible localities? RISC-V supports 5 localities, 4 NTL hint prefixes and 1 without any prefix.

Yes, this is __builtin_prefetch's wrapper. The 4 NTL hint maping had been included in Prefetch Intrinsics part on this doc. So, this commit only support a wrapper for __builtin_prefetch on zicbop extension.

Can you document how the different rw/locality map to different instruction sequences?

Can you document how the different rw/locality map to different instruction sequences?

Do you mean to expand to describe these mappings?
Ok, I will send a new version tomorrow.

yulong18 · 2024-10-26T11:53:15Z

I follow the documentation to set the __riscv_cbo_prefetchi parameter type, but I get such an error that is invalid argument to built-in function. So, I think the authour need to redefine the parameter type.

topperc · 2024-10-26T15:29:24Z

I follow the documentation to set the __riscv_cbo_prefetchi parameter type, but I get such an error that is invalid argument to built-in function. So, I think the authour need to redefine the parameter type.

The rw and locality argument to __builtin_prefetch must be compile time constants. You'll need to use a macro instead of a function.

topperc · 2024-11-25T17:07:45Z

src/c-api.adoc

+----
+void __riscv_cbo_prefetch(void *addr, const int rw, const int locality)
+{
+  __builtin_prefetch(addr, rw, locality);


We need to explain that locality==0, adds nt.all hint. locality==1, adds ntl.pall hint. locality==2, adds ntl.p1 hint.

topperc · 2024-12-19T15:23:31Z

src/c-api.adoc

+|`+void __riscv_cbo_flush(void *addr);+`                                                  |`cbo.flush`          |Zicbom         |
+|`+void __riscv_cbo_inval(void *addr);+`                                                  |`cbo.inval`          |Zicbom         |
+|`+void __riscv_cbo_zero(void *addr);+`                                                   |`cbo.zero`           |Zicboz         |
+|`+void __riscv_cbo_prefetch(void *addr, const int rw, const int locality);+`             |`prefetch.[r][w]`    |Zicbop         | `rw`= [0,1], `locality` = [0..4].


locality is only [0..3] right?

topperc · 2024-12-19T15:28:37Z

src/c-api.adoc

+[%autowidth]
+|===
+|*Prototype*                                                                              |*Instruction*        |*Extension*    |*Notes*
+|`+void __riscv_cbo_clean(void *addr);+`                                                  |`cbo.clean`          |Zicbom         |


The patch that landed in gcc uses __riscv_cmo_clean?

yes，it uses _riscv_cmo* in gcc

cmuellner · 2024-12-20T00:03:20Z

So, we have the situation that the intrinsics landed in GCC about two months ago (d2c8548e0ce51dac6bc51d37236c50f98fca82f0). But we still have valid comments here, which we should take care of.
The good thing is that the recent PR update brought this PR almost into sync with the GCC implementation.

Before going deeper into the details, I'd like to mention that GCC has a generic builtin for prefetching, which also supports locality information in the form of a compile-time constant in the range of zero to three (see https://gcc.gnu.org/onlinedocs/gcc/Other-Builtins.html). A mapping from these arch-independent locality levels to NTL instructions was done in summer 2024 (bf26413fc4081dfd18b915580b35bdb71481327e): 0 -> ntl.all, 1 -> ntl.pall, 2 -> ntl.p1.

The current GCC implementation of __riscv_cmo_prefetch() just invokes the arch-independet __builtin_prefetch() with the same arguments. This means that GCC does currently not support ntl.s1. Further, GCC does not support emitting a prefetch instruction without a preceding NTL instruction if Zihintntl is enabled. In case Zihintntl is not enabled, the locality information is ignored.

I don't think a 1:1 mapping from GCC arch-independent locality encodings to __riscv_cmo_prefetch() is necessary. Instead, I propose the following encoding:

#define __riscv_ntl_undefined 0
#define __riscv_ntl_p1 1
#define __riscv_ntl_pall 2
#define __riscv_ntl_s1 3
#define __riscv_ntl_all 4

Other compile-time constants for the locality parameter of __riscv_cmo_prefetch() should trigger a compile-time error. The behavior of __riscv_cmo_prefetch() could be the same regardless of whether Zihintntl is enabled or not. I.e., if the locality parameter is not zero, we can always emit the hint instruction.

Let me know what you think.

Liaoshihua · 2024-12-20T04:46:28Z

I wrote this proposal with the intention of addressing: https://github.com/riscv-non-isa/riscv-c-api-doc/issues/88，

see https://gcc.gnu.org/onlinedocs/gcc/Other-Builtins.html)

I noticed that GCC has __builtin_prefetch, so __riscv_cmo_prefetch may not be necessary.

#define __riscv_ntl_undefined 0
#define __riscv_ntl_p1 1
#define __riscv_ntl_pall 2
#define __riscv_ntl_s1 3
#define __riscv_ntl_all 4

The following are included in

riscv-c-api-doc/src/c-api.adoc

Line 488 in fbf5f4e

=== NTLH Intrinsics

So, I would remove prefetch in this PR and only include __riscv_cmo_prefetchi, __riscv_cmo_clean, __riscv_cmo_inval, __riscv_cmo_flush, and __riscv_cmo_zero in riscv_cmo.h.

What are your thoughts?

topperc reviewed Oct 25, 2024

View reviewed changes

yulong18 mentioned this pull request Oct 26, 2024

cbo Intrinsic #88

Open

Liaoshihua force-pushed the master branch from 6fa1d14 to 9f44cc9 Compare November 25, 2024 08:11

topperc reviewed Nov 25, 2024

View reviewed changes

Liaoshihua force-pushed the master branch from 9f44cc9 to c092d16 Compare November 26, 2024 13:10

Liaoshihua requested a review from topperc December 19, 2024 13:35

topperc reviewed Dec 19, 2024

View reviewed changes

Liaoshihua force-pushed the master branch 5 times, most recently from a60f03e to 57ee88f Compare December 19, 2024 16:01

Add RISC-V CMO extension's intrinsics

a98843c

Liaoshihua force-pushed the master branch from 57ee88f to a98843c Compare December 19, 2024 16:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add CMO extension intrinsics #93

Add CMO extension intrinsics #93

Liaoshihua commented Oct 25, 2024

topperc Oct 25, 2024

Liaoshihua Oct 30, 2024

topperc Nov 21, 2024

Liaoshihua Nov 21, 2024

yulong18 commented Oct 26, 2024

topperc commented Oct 26, 2024

topperc Nov 25, 2024

topperc Dec 19, 2024

topperc Dec 19, 2024

Liaoshihua Dec 19, 2024

cmuellner commented Dec 20, 2024

Liaoshihua commented Dec 20, 2024 •

edited

Loading

Add CMO extension intrinsics #93

Are you sure you want to change the base?

Add CMO extension intrinsics #93

Conversation

Liaoshihua commented Oct 25, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yulong18 commented Oct 26, 2024

topperc commented Oct 26, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cmuellner commented Dec 20, 2024

Liaoshihua commented Dec 20, 2024 • edited Loading

Liaoshihua commented Dec 20, 2024 •

edited

Loading