-
Notifications
You must be signed in to change notification settings - Fork 42
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add CMO extension intrinsics #93
base: main
Are you sure you want to change the base?
Conversation
src/c-api.adoc
Outdated
|`+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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you document how the different rw/locality map to different instruction sequences?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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. |
src/c-api.adoc
Outdated
---- | ||
void __riscv_cbo_prefetch(void *addr, const int rw, const int locality) | ||
{ | ||
__builtin_prefetch(addr, rw, locality); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to explain that locality==0, adds nt.all
hint. locality==1, adds ntl.pall
hint. locality==2, adds ntl.p1
hint.
src/c-api.adoc
Outdated
|`+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]. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
locality is only [0..3] right?
src/c-api.adoc
Outdated
[%autowidth] | ||
|=== | ||
|*Prototype* |*Instruction* |*Extension* |*Notes* | ||
|`+void __riscv_cbo_clean(void *addr);+` |`cbo.clean` |Zicbom | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The patch that landed in gcc uses __riscv_cmo_clean?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes,it uses _riscv_cmo* in gcc
a60f03e
to
57ee88f
Compare
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. 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): The current GCC implementation of I don't think a 1:1 mapping from GCC arch-independent locality encodings to
Other compile-time constants for the locality parameter of Let me know what you think. |
I wrote this proposal with the intention of addressing: https://github.com/riscv-non-isa/riscv-c-api-doc/issues/88, I noticed that GCC has __builtin_prefetch, so __riscv_cmo_prefetch may not be necessary.
The following are included in riscv-c-api-doc/src/c-api.adoc Line 488 in fbf5f4e
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? |
Add CMO extension intrinsics description.