[PATCH] D111617: [RISCV] Lazily add RVV C intrinsics.

[Fraser Cormack via Phabricator via llvm-commits]

frasercrmck added a comment.

Just nits from me.



================
Comment at: clang/include/clang/Sema/RISCVIntrinsicManager.h:9
+//
+// This file defines the RISCVIntrinsicManager, which handle RISC-V vector
+// intrinsic functions.
----------------
`handles`


================
Comment at: clang/lib/Sema/SemaRISCVVectorLookup.cpp:49
+  // Index of RISCVIntrinsicManagerImpl::IntrinsicList.
+  SmallVector<size_t, 8> Indexs;
+};
----------------
`Indices` (or `Indexes`)?


================
Comment at: clang/lib/Sema/SemaRISCVVectorLookup.cpp:218
+      for (int Log2LMUL = -3; Log2LMUL <= 3; Log2LMUL++) {
+        if (!(Record.Log2LMULMask & (1 << (Log2LMUL + 3)))) {
+          continue;
----------------
Drop curly braces


================
Comment at: clang/lib/Sema/SemaRISCVVectorLookup.cpp:185-186
+                                                   Record.OverloadedSuffixSize);
+    for (int TypeRangeMaskShift = 0;
+         TypeRangeMaskShift <= static_cast<int>(BasicType::MaxOffset);
+         ++TypeRangeMaskShift) {
----------------
aaron.ballman wrote:
> Given that we're bit twiddling with this, I'd feel more comfortable if this was `unsigned int` rather than `int` (same for `BaseTypeI`).
+1


================
Comment at: clang/utils/TableGen/RISCVVEmitter.cpp:47
+
+  // Required extensions for this intrinsic.
+  unsigned RequiredExtension;
----------------
Comment is plural, variable is singular.


================
Comment at: clang/utils/TableGen/RISCVVEmitter.cpp:111
+                           std::vector<SemaRecord> *SemaRecords = nullptr);
+  /// Create all intrinsics record and SemaSignatureTable from SemaRecords.
+  void createRVVIntrinsicRecord(std::vector<RVVIntrinsicRecord> &Out,
----------------
all records, plural?


================
Comment at: clang/utils/TableGen/RISCVVEmitter.cpp:578
+
+    if (SemaRecords) {
+      // Create SemaRecord
----------------
`if (!SemaRecords) continue;`? Might make things a little more readable.


================
Comment at: clang/utils/TableGen/RISCVVEmitter.cpp:637
+    Out.emplace_back(Record);
+    Out.back().Name = SR.Name.c_str();
+    Out.back().OverloadedName = SR.OverloadedName.c_str();
----------------
I assume the compiler's able to avoid recomputing `Out.back()` multiple times? We could take a reference to `Out.back()` and use that, just in case?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111617