[PATCH] D99009: [RISCV] [1/2] Add intrinsic for Zbr extension

[Craig Topper via Phabricator via cfe-commits]

craig.topper added inline comments.


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


================
Comment at: clang/lib/Sema/SemaChecking.cpp:3401
   StringRef Features = Context.BuiltinInfo.getRequiredFeatures(BuiltinID);
-  if (Features.find("experimental-v") != StringRef::npos &&
-      !TI.hasFeature("experimental-v"))
-    return Diag(TheCall->getBeginLoc(), diag::err_riscvv_builtin_requires_v)
-           << TheCall->getSourceRange();
-
-  return false;
+  StringRef(Features).split(ReqFeatures, ',');
+  
----------------
Features is already a StringRef, no need to cast it before calling split.


================
Comment at: clang/lib/Sema/SemaChecking.cpp:3404
+  // Check if each required feature is included in TargetInfo
+  for (size_t i = 0; i < ReqFeatures.size(); i++) {
+    if (!TI.hasFeature(ReqFeatures[i])) {
----------------
Use a range-based for loop.


================
Comment at: clang/lib/Sema/SemaChecking.cpp:3405
+  for (size_t i = 0; i < ReqFeatures.size(); i++) {
+    if (!TI.hasFeature(ReqFeatures[i])) {
+      
----------------
Invert the condition and use a continue; Then we can reduce nesting for the rest of the code.


================
Comment at: clang/lib/Sema/SemaChecking.cpp:3415
+      Diag(TheCall->getBeginLoc(), diag::err_riscv_builtin_requires_extension)
+      << TheCall->getSourceRange() << StringRef(FeatureStr);
+    }
----------------
Please fix formatting as the Pre-merge check suggests.


================
Comment at: llvm/include/llvm/IR/IntrinsicsRISCV.td:29
+    def int_riscv_crc32c_w : BitMan_GPR_Intrinsics;
+    def int_riscv_crc32_d : BitMan_GPR_Intrinsics;
+    def int_riscv_crc32c_d : BitMan_GPR_Intrinsics;
----------------
Put crc32_d directly below crc32_w. So all the crc32_ are together and all the crc32c_ are together.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99009