[PATCH] D99158: [RISCV][WIP] Implement intrinsics for P extension

[Jessica Clarke via Phabricator via cfe-commits]

jrtc27 added inline comments.


================
Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:11170-11175
 // RISC-V V-extension
 def err_riscvv_builtin_requires_v : Error<
    "builtin requires 'V' extension support to be enabled">;
+// RISC-V P-extension
+def err_riscvv_builtin_requires_p : Error<
+   "builtin requires 'P' extension support to be enabled">;
----------------
I seem to recall another patch that includes generalising this to take the extension name rather than adding multiple copies of the same thing


================
Comment at: clang/lib/AST/ASTContext.cpp:4037
+QualType
+ASTContext::getRegisterSizedVectorType(QualType vecType,
+                                       VectorType::VectorKind VecKind) const {
----------------
Upper-case parameter name


================
Comment at: clang/test/CodeGen/builtins-riscv-rv32p.c:63
+
+  // RV32: call i32 @llvm.riscv.add8.i32
+  ul_r = __rv__add8(ul_a, ul_b);
----------------
One function per test and update_cc_test_checks.py would be a lot nicer IMO


================
Comment at: llvm/lib/Target/RISCV/RISCVISelLowering.cpp:715-732
+    if (Subtarget.is64Bit()) {
+      addTypeForP(MVT::v8i8, MVT::i64);
+      addTypeForP(MVT::v4i16, MVT::i64);
+      addTypeForP(MVT::v2i32, MVT::i64);
+    } else {
+      addTypeForP(MVT::v4i8, MVT::i32);
+      addTypeForP(MVT::v2i16, MVT::i32);
----------------
This seems like it will interact poorly with V if both are present


================
Comment at: llvm/test/CodeGen/RISCV/intrinsics-rv32p.ll:5
+
+define void @test() nounwind {
+; RV32P-LABEL: test:
----------------
This is an awful test. One function per intrinsic, and no unnecessary alloca's.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99158