Re: [PATCH 0/2] RISC-V: Define not broken prefetch builtins

[Jeff Law <jeffreyalaw@gmail.com>]

On 9/22/23 01:11, Tsukasa OI wrote:
> Hello,
> 
> As I explained earlier:
> <https://gcc.gnu.org/pipermail/gcc-patches/2023-August/626916.html>,
> the builtin function for RISC-V "__builtin_riscv_zicbop_cbo_prefetchi" is
> completely broken.  Instead, this patch set (in PATCH 1/2) creates three
> new, working builtin intrinsics.
> 
> void __builtin_riscv_prefetch_i(void *addr, [intptr_t offset,] ...);
> void __builtin_riscv_prefetch_r(void *addr, [intptr_t offset,] ...);
> void __builtin_riscv_prefetch_w(void *addr, [intptr_t offset,] ...);
> 
> 
> For consistency with "prefetch.i" and the reason I describe later (which
> requires native instructions for "prefetch.r" and "prefetch.w"), I decided
> to make builtin functions for "prefetch.[rw]" as well.
> 
> Optional second argument (named "offset" here) defaults to zero and must be
> a compile-time integral constant.  Also, it must be a valid offset for a
> "prefetch.[irw]" HINT instruction (x % 32 == 0 && x >= -2048 && x < 2048).
> 
> They are defined if the 'Zicbop' extension is supported and expands to:
> 
>> prefetch.i offset(addr_reg)  ; __builtin_riscv_prefetch_i
>> prefetch.r offset(addr_reg)  ; __builtin_riscv_prefetch_r
>> prefetch.w offset(addr_reg)  ; __builtin_riscv_prefetch_w
> 
> 
> The hardest part of this patch set was to support builtin function with
> variable argument (making "offset" optional).  It required:
> 
> 1.  Support for variable argument function prototype for RISC-V builtins
>      (corresponding "..." on C-based languages)
> 2.  Support for (non-vector) RISC-V builtins with custom expansion
>      (on RVV intrinsics, custom expansion is already implemented)
> 
> 
> ... and PATCH 2/2 fixes an ICE while I'm investigating regular prefetch
> builtin (__builtin_prefetch).  If the 'Zicbop' extension is enabled,
> __builtin_prefetch with the first argument NULL or (not all but) some
> fixed addresses (like ((void*)0x20)) can cause an ICE.  This is because
> the "r" constraint is not checked and a constant can be a first argument
> of target-specific "prefetch" RTL instruction.
> 
> PATCH 2/2 fixes this issue by:
> 
> 1.  Making "prefetch" not an instruction but instead an expansion
>      (this is not rare; e.g. on i386) and
> 2.  Coercing the address argument into a register in the expansion
> 
> It requires separate instructions for "prefetch.[rw]" and I decided to make
> those prefetch instructions very similar to "prefetch.i".  That's one of the
> reasons I created builtins corresponding those.
What I still don't understand is why we're dealing with a decomposed 
address in the builtin, define_expand and/or define_insn.

Have the builtin accept an address, any address.  Then use force_reg to 
force the address into a register in the expander.  My understanding is 
register indirect is always valid.

Create an operand predicate that accepts reg and reg+d for the limited 
displacements allowed.  Use that for the address operand in the 
associated define_insn.


It seems like you're making this more complex than it needs to be.  Or 
I'm missing something critically important.

jeff