[PATCH 0/1] RISC-V: Make "prefetch.i" built-in usable

[PATCH 0/1] RISC-V: Make "prefetch.i" built-in usable

[Tsukasa OI]

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

[PATCH 1/1] RISC-V: Make "prefetch.i" built-in usable

[Tsukasa OI]

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

Re: [PATCH 1/1] RISC-V: Make "prefetch.i" built-in usable

[Jeff Law]

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

Re: [PATCH 1/1] RISC-V: Make "prefetch.i" built-in usable

[Tsukasa OI]

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

Re: [PATCH 1/1] RISC-V: Make "prefetch.i" built-in usable

[Jeff Law]

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