[PATCH] D157510: [RISCV] Implement intrinsics for XCVbitmanip extension in CV32E40P

[Craig Topper via Phabricator via llvm-commits]

craig.topper added a comment.

Is there intended to be a header file that wraps the builtins? For other targets, builtins are considered an internal implementation detail and the real interface is the header file.  I'm trying to get a C interface approved for Zb* and Zk* standard extenions https://github.com/riscv-non-isa/riscv-c-api-doc/pull/44



================
Comment at: llvm/include/llvm/IR/IntrinsicsRISCVXCV.td:14
+class ScalarCoreVBitManipGprGprIntrinsic
+    : Intrinsic<[llvm_i32_ty], [llvm_i32_ty, llvm_i32_ty],
+                [IntrNoMem, IntrWillReturn, IntrSpeculatable]>;
----------------
Use DefaultAttrsIntrinsic. Which I think includes IntrWillReturn


================
Comment at: llvm/include/llvm/IR/IntrinsicsRISCVXCV.td:22
+let TargetPrefix = "riscv" in {
+    def int_riscv_cv_bitmanip_extract : ScalarCoreVBitManipGprGprIntrinsic;
+    def int_riscv_cv_bitmanip_extractu : ScalarCoreVBitManipGprGprIntrinsic;
----------------
Use 2 space indentation


================
Comment at: llvm/include/llvm/IR/IntrinsicsRISCVXCV.td:34
+    def int_riscv_cv_bitmanip_clb : ScalarCoreVBitManipGprIntrinsic;
+    def int_riscv_cv_bitmanip_cnt : ScalarCoreVBitManipGprIntrinsic;
+
----------------
llvm.ctpop?


================
Comment at: llvm/include/llvm/IR/IntrinsicsRISCVXCV.td:36
+
+    def int_riscv_cv_bitmanip_ror : ScalarCoreVBitManipGprGprIntrinsic;
+
----------------
Can we use llvm.fshr like __builtin_rotateright32 does for this?


================
Comment at: llvm/include/llvm/IR/IntrinsicsRISCVXCV.td:38
+
+    def int_riscv_cv_bitmanip_bitrev
+    : Intrinsic<[llvm_i32_ty], [llvm_i32_ty, llvm_i32_ty, llvm_i32_ty],
----------------
Can we use llvm.bitreverse?


================
Comment at: llvm/lib/Target/RISCV/RISCVExpandPseudoInsts.cpp:147
     return expandVMSET_VMCLR(MBB, MBBI, RISCV::VMXNOR_MM);
+  case RISCV::CV_EXTRACT_PSEUDO:
+  case RISCV::CV_EXTRACTU_PSEUDO:
----------------
Can we follow the established convention of having `Pseudo` at the beginning?


================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfoXCV.td:522
+let Predicates = [HasVendorXCVbitmanip, IsRV32] in {
+  defm EXTRACT : PatCoreVBitManip<int_riscv_cv_bitmanip_extract>;
+  defm EXTRACTU : PatCoreVBitManip<int_riscv_cv_bitmanip_extractu>;
----------------
Why do we need these pseudos? Looks like it just to split up the immediate? Can't we do that with an SDNodeXForm in tablegen?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157510