* [PATCH 0/1] RISC-V: Make "prefetch.i" built-in usable @ 2023-08-10 3:10 Tsukasa OI 2023-08-10 3:10 ` [PATCH 1/1] " Tsukasa OI 0 siblings, 1 reply; 5+ messages in thread From: Tsukasa OI @ 2023-08-10 3:10 UTC (permalink / raw) To: Tsukasa OI, Kito Cheng, Palmer Dabbelt, Andrew Waterman, Jim Wilson Cc: gcc-patches Hello, I found that a built-in function "__builtin_riscv_zicbop_cbo_prefetchi" is terribly broken so that this is practically unusable. It emits the "prefetch.i" machine instruction HINT but with no usable arguments. Contents of a.c: > void function_to_be_called(void); > > void sample(void) > { > __builtin_riscv_zicbop_cbo_prefetchi(&function_to_be_called); > function_to_be_called(); > } Compiling with the 'Zicbop' extension fails. > $ riscv64-unknown-elf-gcc -O2 -march=rv64i_zicbop -mabi=lp64 -c a.c > a.c: In function 'sample': > a.c:5:42: warning: passing argument 1 of '__builtin_riscv_zicbop_cbo_prefetchi' makes integer from pointer without a cast [-Wint-conversion] > 5 | __builtin_riscv_zicbop_cbo_prefetchi(&function_to_be_called); > | ^~~~~~~~~~~~~~~~~~~~~~ > | | > | void (*)(void) > a.c:5:42: note: expected 'long int' but argument is of type 'void (*)(void)' > a.c:5:5: error: invalid argument to built-in function > 5 | __builtin_riscv_zicbop_cbo_prefetchi(&function_to_be_called); > | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ seems the prototype of this built-in function is... > int __builtin_riscv_zicbop_cbo_prefetchi(int); // RV32 > long __builtin_riscv_zicbop_cbo_prefetchi(long); // RV64 I started to have a bad feeling about this. Looking at the test case, following code compiles (sort of). > void sample(void) > { > __builtin_riscv_zicbop_cbo_prefetchi(1); /* integer constant: 0-4 (inclusive) */ > } WHAT IS THIS PREFETCHING!? The answer was obvious (tested with GCC 13.2.0). > $ riscv64-unknown-elf-gcc -O2 -march=rv64i_zicbop -mabi=lp64 -S a.c > $ cat a.s > .file "a.c" > .option nopic > .attribute arch, "rv64i2p1_zicbop1p0" > .attribute unaligned_access, 0 > .attribute stack_align, 16 > .text > .align 2 > .globl sample > .type sample, @function > sample: > li a5,0 > prefetch.i 0(a5) > ret > .size sample, .-sample > .ident "GCC: (gc891d8dc23e1) 13.2.0" ... none (NULL). Even the prefetching constant (1) is ignored. It makes no sense whatsoever. This patch set is definitely compatibility- breaking but necessary to make "prefetch.i" work. This patch set makes following built-in function. > void __builtin_riscv_zicbop_prefetch_i(void*); Did you notice that I renamed the function? There are three reasons: 1. We needed to break the compatibility anyway. 2. To avoid functionality problems. Co-existing functional and non-functional __builtin_riscv_zicbop_cbo_prefetchi built-in felt like a nightmare. 3. Although this is also a cache block operation, the instruction "prefetch.i" does not have the name "cbo" in it (unlike "__builtin_riscv_zicbom_cbo_clean" corresponding "cbo.clean"). Let's make a working sample (a.c): > void function_to_be_called(void); > > void sample(void) > { > __builtin_riscv_zicbop_prefetch_i(&function_to_be_called); > function_to_be_called(); > } And take look at the output assembly and its relocations. > $ riscv64-unknown-elf-gcc -O2 -march=rv64i_zicbop -mabi=lp64 -S a.c > $ cat a.s > .file "a.c" > .option nopic > .attribute arch, "rv64i2p1_zicbop1p0" > .attribute unaligned_access, 0 > .attribute stack_align, 16 > .text > .align 2 > .globl sample > .type sample, @function > sample: > lui a5,%hi(function_to_be_called) > addi a5,a5,%lo(function_to_be_called) > prefetch.i 0(a5) > tail function_to_be_called > .size sample, .-sample > .ident "GCC: (GNU) 14.0.0 20230810 (experimental)" I hope this issue is fixed soon. Sincerely, Tsukasa Tsukasa OI (1): RISC-V: Make "prefetch.i" built-in usable gcc/config/riscv/riscv-cmo.def | 4 ++-- gcc/config/riscv/riscv.md | 5 ++--- gcc/doc/extend.texi | 7 +++++++ gcc/testsuite/gcc.target/riscv/cmo-zicbop-1.c | 8 +++++--- gcc/testsuite/gcc.target/riscv/cmo-zicbop-2.c | 10 ++++++---- 5 files changed, 22 insertions(+), 12 deletions(-) base-commit: 9b099a83b45b8fcdfc07d518e05d36ea741b2227 -- 2.41.0 ^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH 1/1] RISC-V: Make "prefetch.i" built-in usable 2023-08-10 3:10 [PATCH 0/1] RISC-V: Make "prefetch.i" built-in usable Tsukasa OI @ 2023-08-10 3:10 ` Tsukasa OI 2023-08-28 21:20 ` Jeff Law 0 siblings, 1 reply; 5+ messages in thread From: Tsukasa OI @ 2023-08-10 3:10 UTC (permalink / raw) To: Tsukasa OI, Kito Cheng, Palmer Dabbelt, Andrew Waterman, Jim Wilson Cc: gcc-patches From: Tsukasa OI <research_trasio@irq.a4lg.com> The "__builtin_riscv_zicbop_cbo_prefetchi" built-in function was terribly broken so that practically unusable. It emitted "prefetch.i" but with no meaningful arguments. Though incompatible, this commit completely changes the function prototype of this built-in and makes it usable. To minimize the functionality issues, it renames the built-in to "__builtin_riscv_zicbop_prefetch_i". gcc/ChangeLog: * config/riscv/riscv-cmo.def: Fix function prototype. * config/riscv/riscv.md (riscv_prefetchi_<mode>): Fix instruction prototype. Remove possible prefectch type argument * doc/extend.texi: Document __builtin_riscv_zicbop_prefetch_i. gcc/testsuite/ChangeLog: * gcc.target/riscv/cmo-zicbop-1.c: Reflect new built-in prototype. * gcc.target/riscv/cmo-zicbop-2.c: Likewise. --- gcc/config/riscv/riscv-cmo.def | 4 ++-- gcc/config/riscv/riscv.md | 5 ++--- gcc/doc/extend.texi | 7 +++++++ gcc/testsuite/gcc.target/riscv/cmo-zicbop-1.c | 8 +++++--- gcc/testsuite/gcc.target/riscv/cmo-zicbop-2.c | 10 ++++++---- 5 files changed, 22 insertions(+), 12 deletions(-) diff --git a/gcc/config/riscv/riscv-cmo.def b/gcc/config/riscv/riscv-cmo.def index b92044dc6ff9..2286c8a25544 100644 --- a/gcc/config/riscv/riscv-cmo.def +++ b/gcc/config/riscv/riscv-cmo.def @@ -13,8 +13,8 @@ RISCV_BUILTIN (zero_si, "zicboz_cbo_zero", RISCV_BUILTIN_DIRECT_NO_TARGET, RISCV RISCV_BUILTIN (zero_di, "zicboz_cbo_zero", RISCV_BUILTIN_DIRECT_NO_TARGET, RISCV_VOID_FTYPE_VOID_PTR, zero64), // zicbop -RISCV_BUILTIN (prefetchi_si, "zicbop_cbo_prefetchi", RISCV_BUILTIN_DIRECT, RISCV_SI_FTYPE_SI, prefetchi32), -RISCV_BUILTIN (prefetchi_di, "zicbop_cbo_prefetchi", RISCV_BUILTIN_DIRECT, RISCV_DI_FTYPE_DI, prefetchi64), +RISCV_BUILTIN (prefetchi_si, "zicbop_prefetch_i", RISCV_BUILTIN_DIRECT_NO_TARGET, RISCV_VOID_FTYPE_VOID_PTR, prefetchi32), +RISCV_BUILTIN (prefetchi_di, "zicbop_prefetch_i", RISCV_BUILTIN_DIRECT_NO_TARGET, RISCV_VOID_FTYPE_VOID_PTR, prefetchi64), // zbkc or zbc RISCV_BUILTIN (clmul_si, "clmul", RISCV_BUILTIN_DIRECT, RISCV_SI_FTYPE_SI_SI, clmul_zbkc32_or_zbc32), diff --git a/gcc/config/riscv/riscv.md b/gcc/config/riscv/riscv.md index 688fd697255b..5658c7b7e113 100644 --- a/gcc/config/riscv/riscv.md +++ b/gcc/config/riscv/riscv.md @@ -3273,9 +3273,8 @@ }) (define_insn "riscv_prefetchi_<mode>" - [(unspec_volatile:X [(match_operand:X 0 "address_operand" "r") - (match_operand:X 1 "imm5_operand" "i")] - UNSPECV_PREI)] + [(unspec_volatile:X [(match_operand:X 0 "register_operand" "r")] + UNSPECV_PREI)] "TARGET_ZICBOP" "prefetch.i\t%a0" ) diff --git a/gcc/doc/extend.texi b/gcc/doc/extend.texi index 89c5b4ea2b20..0eb98fc89e3f 100644 --- a/gcc/doc/extend.texi +++ b/gcc/doc/extend.texi @@ -21575,6 +21575,13 @@ Xgnuzihintpausestate extension, which redefines the @code{pause} instruction to change architectural state. @enddefbuiltin +@defbuiltin{void __builtin_riscv_zicbop_prefetch_i (void *@var{addr})} +Generates the @code{prefetch.i} machine instruction to instruct the hardware +that a cache block containing @var{addr} is likely to be accessed by an +instruction fetch in the near future. +Available if the Zicbop extension is enabled. +@enddefbuiltin + @node RISC-V Vector Intrinsics @subsection RISC-V Vector Intrinsics diff --git a/gcc/testsuite/gcc.target/riscv/cmo-zicbop-1.c b/gcc/testsuite/gcc.target/riscv/cmo-zicbop-1.c index c5d78c1763d3..0d5b58c4fadf 100644 --- a/gcc/testsuite/gcc.target/riscv/cmo-zicbop-1.c +++ b/gcc/testsuite/gcc.target/riscv/cmo-zicbop-1.c @@ -13,11 +13,13 @@ void foo (char *p) __builtin_prefetch (p, 1, 3); } -int foo1() +void foo1 () { - return __builtin_riscv_zicbop_cbo_prefetchi(1); + __builtin_riscv_zicbop_prefetch_i(0); + __builtin_riscv_zicbop_prefetch_i(&foo); + __builtin_riscv_zicbop_prefetch_i((void*)0x111); } -/* { dg-final { scan-assembler-times "prefetch.i" 1 } } */ +/* { dg-final { scan-assembler-times "prefetch.i" 3 } } */ /* { dg-final { scan-assembler-times "prefetch.r" 4 } } */ /* { dg-final { scan-assembler-times "prefetch.w" 4 } } */ diff --git a/gcc/testsuite/gcc.target/riscv/cmo-zicbop-2.c b/gcc/testsuite/gcc.target/riscv/cmo-zicbop-2.c index 6576365b39ca..09655c4b8593 100644 --- a/gcc/testsuite/gcc.target/riscv/cmo-zicbop-2.c +++ b/gcc/testsuite/gcc.target/riscv/cmo-zicbop-2.c @@ -13,11 +13,13 @@ void foo (char *p) __builtin_prefetch (p, 1, 3); } -int foo1() +void foo1 () { - return __builtin_riscv_zicbop_cbo_prefetchi(1); + __builtin_riscv_zicbop_prefetch_i(0); + __builtin_riscv_zicbop_prefetch_i(&foo); + __builtin_riscv_zicbop_prefetch_i((void*)0x111); } -/* { dg-final { scan-assembler-times "prefetch.i" 1 } } */ +/* { dg-final { scan-assembler-times "prefetch.i" 3 } } */ /* { dg-final { scan-assembler-times "prefetch.r" 4 } } */ -/* { dg-final { scan-assembler-times "prefetch.w" 4 } } */ +/* { dg-final { scan-assembler-times "prefetch.w" 4 } } */ -- 2.41.0 ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH 1/1] RISC-V: Make "prefetch.i" built-in usable 2023-08-10 3:10 ` [PATCH 1/1] " Tsukasa OI @ 2023-08-28 21:20 ` Jeff Law 2023-08-29 2:09 ` Tsukasa OI 0 siblings, 1 reply; 5+ messages in thread From: Jeff Law @ 2023-08-28 21:20 UTC (permalink / raw) To: Tsukasa OI, Kito Cheng, Palmer Dabbelt, Andrew Waterman, Jim Wilson Cc: gcc-patches On 8/9/23 21:10, Tsukasa OI via Gcc-patches wrote: > From: Tsukasa OI <research_trasio@irq.a4lg.com> > > The "__builtin_riscv_zicbop_cbo_prefetchi" built-in function was terribly > broken so that practically unusable. It emitted "prefetch.i" but with no > meaningful arguments. > > Though incompatible, this commit completely changes the function prototype > of this built-in and makes it usable. To minimize the functionality issues, > it renames the built-in to "__builtin_riscv_zicbop_prefetch_i". > > gcc/ChangeLog: > > * config/riscv/riscv-cmo.def: Fix function prototype. > * config/riscv/riscv.md (riscv_prefetchi_<mode>): Fix instruction > prototype. Remove possible prefectch type argument > * doc/extend.texi: Document __builtin_riscv_zicbop_prefetch_i. > > gcc/testsuite/ChangeLog: > > * gcc.target/riscv/cmo-zicbop-1.c: Reflect new built-in prototype. > * gcc.target/riscv/cmo-zicbop-2.c: Likewise. > diff --git a/gcc/config/riscv/riscv.md b/gcc/config/riscv/riscv.md > index 688fd697255b..5658c7b7e113 100644 > --- a/gcc/config/riscv/riscv.md > +++ b/gcc/config/riscv/riscv.md > @@ -3273,9 +3273,8 @@ > }) > > (define_insn "riscv_prefetchi_<mode>" > - [(unspec_volatile:X [(match_operand:X 0 "address_operand" "r") > - (match_operand:X 1 "imm5_operand" "i")] > - UNSPECV_PREI)] > + [(unspec_volatile:X [(match_operand:X 0 "register_operand" "r")] > + UNSPECV_PREI)] > "TARGET_ZICBOP" > "prefetch.i\t%a0" > ) What I would suggest is making a new predicate that accepts either a register or a register+offset where the offset fits in a signed 12 bit immediate. Use that for operand 0's predicate and I think this will "just work" and cover all the cases supported by the prefetch.i instruction. Jeff ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH 1/1] RISC-V: Make "prefetch.i" built-in usable 2023-08-28 21:20 ` Jeff Law @ 2023-08-29 2:09 ` Tsukasa OI 2023-08-29 13:47 ` Jeff Law 0 siblings, 1 reply; 5+ messages in thread From: Tsukasa OI @ 2023-08-29 2:09 UTC (permalink / raw) To: Jeff Law, Kito Cheng, Palmer Dabbelt, Andrew Waterman, Jim Wilson Cc: gcc-patches On 2023/08/29 6:20, Jeff Law wrote: > > > On 8/9/23 21:10, Tsukasa OI via Gcc-patches wrote: >> From: Tsukasa OI <research_trasio@irq.a4lg.com> >> >> The "__builtin_riscv_zicbop_cbo_prefetchi" built-in function was terribly >> broken so that practically unusable. It emitted "prefetch.i" but with no >> meaningful arguments. >> >> Though incompatible, this commit completely changes the function >> prototype >> of this built-in and makes it usable. To minimize the functionality >> issues, >> it renames the built-in to "__builtin_riscv_zicbop_prefetch_i". >> >> gcc/ChangeLog: >> >> * config/riscv/riscv-cmo.def: Fix function prototype. >> * config/riscv/riscv.md (riscv_prefetchi_<mode>): Fix instruction >> prototype. Remove possible prefectch type argument >> * doc/extend.texi: Document __builtin_riscv_zicbop_prefetch_i. >> >> gcc/testsuite/ChangeLog: >> >> * gcc.target/riscv/cmo-zicbop-1.c: Reflect new built-in prototype. >> * gcc.target/riscv/cmo-zicbop-2.c: Likewise. >> diff --git a/gcc/config/riscv/riscv.md b/gcc/config/riscv/riscv.md >> index 688fd697255b..5658c7b7e113 100644 >> --- a/gcc/config/riscv/riscv.md >> +++ b/gcc/config/riscv/riscv.md >> @@ -3273,9 +3273,8 @@ >> }) >> (define_insn "riscv_prefetchi_<mode>" >> - [(unspec_volatile:X [(match_operand:X 0 "address_operand" "r") >> - (match_operand:X 1 "imm5_operand" "i")] >> - UNSPECV_PREI)] >> + [(unspec_volatile:X [(match_operand:X 0 "register_operand" "r")] >> + UNSPECV_PREI)] >> "TARGET_ZICBOP" >> "prefetch.i\t%a0" >> ) > What I would suggest is making a new predicate that accepts either a > register or a register+offset where the offset fits in a signed 12 bit > immediate. Use that for operand 0's predicate and I think this will > "just work" and cover all the cases supported by the prefetch.i > instruction. > > Jeff > Seems reasonable. If we have to break the compatibility anyway, adding an offset argument is not a bad idea (though I think they will use inline assembly if a non-zero offset is required). I will try to add *optional* offset argument (with default value 0) like __builtin_speculation_safe_value built-in function in the next version. Thanks, Tsukasa ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH 1/1] RISC-V: Make "prefetch.i" built-in usable 2023-08-29 2:09 ` Tsukasa OI @ 2023-08-29 13:47 ` Jeff Law 0 siblings, 0 replies; 5+ messages in thread From: Jeff Law @ 2023-08-29 13:47 UTC (permalink / raw) To: Tsukasa OI, Kito Cheng, Palmer Dabbelt, Andrew Waterman, Jim Wilson Cc: gcc-patches On 8/28/23 20:09, Tsukasa OI wrote: > On 2023/08/29 6:20, Jeff Law wrote: >> >> >> On 8/9/23 21:10, Tsukasa OI via Gcc-patches wrote: >>> From: Tsukasa OI <research_trasio@irq.a4lg.com> >>> >>> The "__builtin_riscv_zicbop_cbo_prefetchi" built-in function was terribly >>> broken so that practically unusable. It emitted "prefetch.i" but with no >>> meaningful arguments. >>> >>> Though incompatible, this commit completely changes the function >>> prototype >>> of this built-in and makes it usable. To minimize the functionality >>> issues, >>> it renames the built-in to "__builtin_riscv_zicbop_prefetch_i". >>> >>> gcc/ChangeLog: >>> >>> * config/riscv/riscv-cmo.def: Fix function prototype. >>> * config/riscv/riscv.md (riscv_prefetchi_<mode>): Fix instruction >>> prototype. Remove possible prefectch type argument >>> * doc/extend.texi: Document __builtin_riscv_zicbop_prefetch_i. >>> >>> gcc/testsuite/ChangeLog: >>> >>> * gcc.target/riscv/cmo-zicbop-1.c: Reflect new built-in prototype. >>> * gcc.target/riscv/cmo-zicbop-2.c: Likewise. >>> diff --git a/gcc/config/riscv/riscv.md b/gcc/config/riscv/riscv.md >>> index 688fd697255b..5658c7b7e113 100644 >>> --- a/gcc/config/riscv/riscv.md >>> +++ b/gcc/config/riscv/riscv.md >>> @@ -3273,9 +3273,8 @@ >>> }) >>> (define_insn "riscv_prefetchi_<mode>" >>> - [(unspec_volatile:X [(match_operand:X 0 "address_operand" "r") >>> - (match_operand:X 1 "imm5_operand" "i")] >>> - UNSPECV_PREI)] >>> + [(unspec_volatile:X [(match_operand:X 0 "register_operand" "r")] >>> + UNSPECV_PREI)] >>> "TARGET_ZICBOP" >>> "prefetch.i\t%a0" >>> ) >> What I would suggest is making a new predicate that accepts either a >> register or a register+offset where the offset fits in a signed 12 bit >> immediate. Use that for operand 0's predicate and I think this will >> "just work" and cover all the cases supported by the prefetch.i >> instruction. >> >> Jeff >> > > Seems reasonable. > > If we have to break the compatibility anyway, adding an offset argument > is not a bad idea (though I think they will use inline assembly if a > non-zero offset is required). > > I will try to add *optional* offset argument (with default value 0) like > __builtin_speculation_safe_value built-in function in the next version. The reason I suggested creating a predicate which allowed either a reg or reg+offset was to give that level of flexibility. The inline assembly would specify an address. If the address was already in a register, great. If the address is expressable as reg+offset, also great. If the address was a symbolic operand, the right predicate+constraint should force the symbolic into a register. Jeff ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2023-08-29 13:47 UTC | newest] Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2023-08-10 3:10 [PATCH 0/1] RISC-V: Make "prefetch.i" built-in usable Tsukasa OI 2023-08-10 3:10 ` [PATCH 1/1] " Tsukasa OI 2023-08-28 21:20 ` Jeff Law 2023-08-29 2:09 ` Tsukasa OI 2023-08-29 13:47 ` Jeff Law
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 read-only IMAP folder(s) and NNTP newsgroup(s).