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

[Zakk Chen via Phabricator via cfe-commits]

khchen added inline comments.


================
Comment at: clang/lib/Sema/SemaRVVLookup.cpp:175
+  for (auto &Record : RVVIntrinsicRecords) {
+    // Create Intrinsics for each type and LMUL.
+    BasicType BaseType = BasicType::Unknown;
----------------
Those code logic need to sync with createRVVIntrinsics, maybe we could add more comment address that.


================
Comment at: clang/utils/TableGen/RISCVVEmitter.cpp:91
 private:
   /// Create all intrinsics and add them to \p Out
   void createRVVIntrinsics(std::vector<std::unique_ptr<RVVIntrinsic>> &Out);
----------------
and also init SemaRecords?


================
Comment at: clang/utils/TableGen/RISCVVEmitter.cpp:93
   void createRVVIntrinsics(std::vector<std::unique_ptr<RVVIntrinsic>> &Out);
+  /// Create all intrinsics record from RVVIntrinsics.
+  void createRVVIntrinsicRecord(std::vector<RVVIntrinsicRecord> &Out);
----------------
I think it should be "Create all intrinsics record and SemaSignatureTable from SemaRecords"?


================
Comment at: clang/utils/TableGen/RISCVVEmitter.cpp:99
 
-  /// Emit Acrh predecessor definitions and body, assume the element of Defs are
-  /// sorted by extension.
-  void emitArchMacroAndBody(
-      std::vector<std::unique_ptr<RVVIntrinsic>> &Defs, raw_ostream &o,
-      std::function<void(raw_ostream &, const RVVIntrinsic &)>);
-
-  // Emit the architecture preprocessor definitions. Return true when emits
-  // non-empty string.
-  bool emitMacroRestrictionStr(RISCVPredefinedMacroT PredefinedMacros,
-                               raw_ostream &o);
+  /// Construct a compressed signature table used for createSema.
+  void ConstructSemaSignatureTable();
----------------
/// Construct a compressed signature table from SemaRecords which is used for createSema.
maybe better.



================
Comment at: clang/utils/TableGen/RISCVVEmitter.cpp:103
+  unsigned
+  GetSemaSignatureIndex(const SmallVector<PrototypeDescriptor> &Signature);
 };
----------------
This is ambiguous of naming and comment because it also insert signature into SemaSignatureTable if not found in the table.


================
Comment at: clang/utils/TableGen/RISCVVEmitter.cpp:597
+      };
+
+  for (const auto &SemaRecord : SemaRecords) {
----------------
maybe we need an assert to check SemaRecords is not empty.


================
Comment at: clang/utils/TableGen/RISCVVEmitter.cpp:637
+  std::vector<RVVIntrinsicRecord> RVVIntrinsicRecords;
+  createRVVIntrinsics(Defs);
+
----------------
we only need SemaRecords initialization part of createRVVIntrinsics, right?


================
Comment at: clang/utils/TableGen/RISCVVEmitter.cpp:639
+
+  createRVVIntrinsicRecord(RVVIntrinsicRecords);
+
----------------
`createRVVIntrinsicRecord also init SemaSignatureTable implicitly.`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111617