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

[Jessica Clarke via Phabricator via cfe-commits]

jrtc27 added a comment.

Please fix the style issues and update the patch with full context (see https://llvm.org/docs/Phabricator.html#requesting-a-review-via-the-web-interface, or use `arc`)



================
Comment at: clang/include/clang/Basic/BuiltinsRISCV.def:20
 
+// zbr extension
+TARGET_BUILTIN(__builtin_riscv_crc32_b, "LiLi", "nc", "experimental-zbr")
----------------
Zbr


================
Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:11170
 
-// RISC-V V-extension
-def err_riscvv_builtin_requires_v : Error<
-   "builtin requires 'V' extension support to be enabled">;
+// RISC-V experimental extension
+def err_riscv_builtin_requires_extension : Error<
----------------
This will apply to non-experimental extensions too once V/B/K/etc get ratified


================
Comment at: clang/lib/Sema/SemaChecking.cpp:3400
+  
+  if (!TI.hasFeature(Features))
+  {
----------------
This new code assumes there's only one feature in the string


================
Comment at: clang/lib/Sema/SemaChecking.cpp:3405
+           << TheCall->getSourceRange()
+           << Features;
+  }
----------------
Extension name should have an upper case first letter


================
Comment at: clang/test/CodeGen/RISCV/rvb-intrinsics/riscv32-zbr.c:15-16
+//
+long crc32b(long a)
+{
+  return __builtin_riscv_crc32_b(a);
----------------
`long crc32b(long a) {` for all these


================
Comment at: clang/test/Headers/rvintrin.c:5
+// RUN:   -target-feature +experimental-v %s
+// expected-no-diagnostics
+
----------------
This does nothing unless you add -verify to the Clang command line


================
Comment at: llvm/include/llvm/IR/IntrinsicsRISCV.td:20
+    class BitMan_GPR_Intrinsics
+        : Intrinsic<[llvm_any_ty],[llvm_any_ty],
+                    [IntrNoMem, IntrSpeculatable, IntrWillReturn]>;
----------------
Space after comma


================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfoB.td:879
+let Predicates = [HasStdExtZbr] in {
+def : PatGpr<int_riscv_crc32_b,CRC32B>;
+def : PatGpr<int_riscv_crc32_h,CRC32H>;
----------------
Space after comma


================
Comment at: llvm/test/CodeGen/RISCV/rv32Zbr.ll:1
+; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
+; RUN: llc -mtriple=riscv32 -mattr=experimental-zbr -verify-machineinstrs < %s \
----------------
Lowercase z in the names of these files as it's a (sort of) -march string


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99009