[PATCH] D99319: [RISCV] [2/2] Add intrinsic for Zbb extension

[Craig Topper via Phabricator via cfe-commits]

craig.topper added inline comments.


================
Comment at: clang/lib/Headers/riscv_zbb_intrin.h:18
+// Zbb
+static __inline__ int_xlen_t __DEFAULT_FN_ATTRS _rv_orc_b(int_xlen_t rs1) {
+  return __builtin_riscv_orc_b(rs1);
----------------
I think rather than int_xlen_t we want an int32_t version for RV32/RV64 and an int64_t version for RV64. I think it is safe to use the 64-bit orc.b instruction for int32_t on RV64 since each output byte is only affected by the bits in the same input byte. So we can put garbage in the upper 4 bytes and ignore the results in the upper bytes.

We couldn't do this for crc32(c) from Zbr.


================
Comment at: clang/lib/Sema/SemaChecking.cpp:3398
   // message.
+  bool miss_feature_error = false;
+  SmallVector<StringRef> ReqFeatures;
----------------
Variable names should start with capital letter and use CamelCase. No underscores.


================
Comment at: clang/lib/Sema/SemaChecking.cpp:3415
+      Diag(TheCall->getBeginLoc(), diag::err_riscv_builtin_requires_extension)
+      << TheCall->getSourceRange() << StringRef(FeatureStr);
+    }
----------------
Please run clang-format on this line. There are instructions here for running clang-format on a patch https://clang.llvm.org/docs/ClangFormat.html#script-for-patch-reformatting


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99319