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

[Jeff Law <jeffreyalaw@gmail.com>]

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