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

[Tsukasa OI <research_trasio@irq.a4lg.com>]

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