[PATCH] D93359: [RISCV] Define vle/vse intrinsics.

[Zakk Chen via Phabricator via llvm-commits]

khchen marked 3 inline comments as done.
khchen added inline comments.


================
Comment at: llvm/include/llvm/IR/IntrinsicsRISCV.td:87
+                    [LLVMPointerType<LLVMMatchType<0>>,
+                     llvm_anyint_ty],
+                    [NoCapture<ArgIndex<0>>, IntrReadMem]>, RISCVVIntrinsic;
----------------
craig.topper wrote:
> liaolucy wrote:
> > There are two variants of intrinsics regarding to vl, https://github.com/riscv/rvv-intrinsic-doc/blob/master/rvv-intrinsic-rfc.md#vl-argument
> > Any suggestions for implementing implicit vl intrinsics? What existing code can be reused?
> We've been modelling this in our codebase by passing a readvl intrinsic to the vl argument of the intrinsics defined here. But that's interfering with some optimizations.
> 
> I'm not sure what the best way to implement the intrinsics that don't take a vl argument is. Since the middle end can't model the data flow of a hidden VL register we would need to mark all the intrinsics that use a hidden vl as "having side effects" so they don't get reordered around vsetvl intrinsics. The side effect flag will also prevent optimization. It also means you can't mix intrinsics that take a vl argument with intrinsics that use the current vl without also marking the intrinsics that take vl argument as having side effects.
Yes, just like Craig's comment, we use a wrapper as below to model implict vl intrinsics, but there is a performance issue in this approach. 

```
#define vadd_vv_i32m8_vl(...) __builtin_rvv_vadd_vv_i32m8_vl(__VA_ARGS__)

#define vadd_vv_i32m8(op0, op1) __builtin_rvv_vadd_vv_i32m8_vl( \
(__rvv_int32m8_t)(op0), \
(__rvv_int32m8_t)(op1), \
(size_t)(__builtin_rvv_vreadvl()))
``` 

intrinsic spec does not force implementation must support both approaches (explicit or Implicit), so I think maybe start with supporting explicit vl api is fine. 


================
Comment at: llvm/include/llvm/IR/IntrinsicsRISCV.td:205
+  foreach eew = [8, 16, 32, 64] in {
+    defm "vle" # eew: RISCVUSLoad;
+    defm "vse" # eew: RISCVUSStore;
----------------
craig.topper wrote:
> Do we really need a separate intrinsic for each eew? Can't we infer eew from the element type of the result?
fixed, thanks!


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D93359/new/

https://reviews.llvm.org/D93359