[PATCH] D93013: [RISCV] Define vadd intrinsics and lower to V instructions.

[Craig Topper via Phabricator via llvm-commits]

craig.topper added inline comments.


================
Comment at: llvm/include/llvm/IR/IntrinsicsRISCV.td:26
 
+class RISCVVIntrinsic {
+  // These intrinsics may accept illegal integer values in their llvm_any_ty
----------------
The comment block above this is about atomics which should stay with the atomic intrinsics. Can we start a new section for vectors after closing the "let TargetPrefix = "riscv" in {" portion for atomics. Same as what I did in D92973.


================
Comment at: llvm/include/llvm/IR/IntrinsicsRISCV.td:81
+        : Intrinsic<[llvm_anyvector_ty],
+                    [LLVMMatchType<0>, llvm_any_ty, llvm_i64_ty],
+                    [IntrNoMem]>, RISCVVIntrinsic {
----------------
The vl parameter is going to need to be llvm_anyint_ty to support RV32


================
Comment at: llvm/lib/Target/RISCV/RISCVISelLowering.cpp:2119
                      ISD::ArgFlagsTy ArgFlags, CCState &State, bool IsFixed,
-                     bool IsRet, Type *OrigTy) {
+                     bool IsRet, Type *OrigTy, const RISCVTargetLowering *TLI) {
   unsigned XLen = DL.getLargestLegalIntTypeSizeInBits();
----------------
Can you pass TLI by reference since its never null? Just need to change the callers to pass *this instead of this


================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfoVPseudos.td:73
 
+// We only model FPR32 for V instructions in RISCVInstrInfoV.td.
+// FP16/FP32/FP64 registers are alias each other. Convert FPR16 and FPR64
----------------
Why do we need to do this? The vector instructions for that use a FPR64 or FPR32 should only be supported when the scalar type is supported. Why can't we use the proper register class directly?


================
Comment at: llvm/lib/Target/RISCV/Utils/RISCVBaseInfo.cpp:17
 #include "llvm/ADT/Triple.h"
+#include "llvm/IR/IntrinsicsRISCV.h"
 #include "llvm/Support/raw_ostream.h"
----------------
This feels like a library layering violation. This file belongs to the MC layer and shouldn't be pulling in files in the IR directory.


================
Comment at: llvm/test/CodeGen/RISCV/rvv/vadd.ll:18
+entry:
+  %a = call <vscale x 1 x i8> @llvm.riscv.vadd.nxv1i8.nxv1i8(
+    <vscale x 1 x i8> undef,
----------------
frasercrmck wrote:
> It would be good to have a test without `undef` inputs.
Agreed. Can we just avoid undef inputs in all tests?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93013