[PATCH] D98388: [RISCV][Clang] Add RVV vle/vse intrinsic functions.

[Roger Ferrer Ibanez via Phabricator via cfe-commits]

rogfer01 added a comment.

Overall LGTM. Thanks @khchen!



================
Comment at: clang/include/clang/Basic/riscv_vector.td:175
+  // builtin to C/C++. It is parameter of the unmasked version without VL
+  // operand.
+  list<int> PermuteOperands = [];
----------------
Not sure if we want to clarify that when this list is empty, the permutation is assumed to be equivalent to `[0, 1, 2, 3, ...]`.


================
Comment at: clang/include/clang/Basic/riscv_vector.td:224
+        IntrinsicTypes = {ResultType, Ops[1]->getType()};
+        Ops[0] = Builder.CreateBitCast(Ops[0],
+        llvm::PointerType::getUnqual(ResultType)); }],
----------------
I think you may want to indent this, and then lines 228 and 248 like you indented line 252.


================
Comment at: clang/utils/TableGen/RISCVVEmitter.cpp:689
+      skew = 1;
+    for (unsigned i = 0; i < PermuteOperands.size(); ++i) {
+      if (i != PermuteOperands[i])
----------------
These are only suggestions of sanity checks we could do here:
- I understand `PermuteOperand.size()` should be `<=` than `CTypeOrder.size()`. 
- Also `PermuteOperands[i] + skew` should be `<` than `CTypeOrder.size()`. right?
- We could check the result is indeed a permutation (e.g. sorting a copy of `CTypeOrder` is equivalent to the iota above). This one might be expensive although the sequences are short, not sure.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98388