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

[Zakk Chen via Phabricator via llvm-commits]

khchen added a comment.
Herald added a subscriber: eopXD.

It seems like not only one place need to have a consistent way to process intrinsic. (ex. InitIntrinsicList/createRVVIntrinsics and RVVIntrinsic::RVVIntrinsic/InitRVVIntrinsic)
I'm think how to avoid mismatch implementation in the future, because we will support a lot of new builtin with tail and mask policy...



================
Comment at: clang/lib/Sema/SemaRVVLookup.cpp:91
+struct RVVIntrinsicDef {
+  std::string Name;
+  std::string GenericName;
----------------
why do we need to declare Name as std::string here but RVVIntrinsicRecord use `const char*`?


================
Comment at: clang/lib/Sema/SemaRVVLookup.cpp:92
+  std::string Name;
+  std::string GenericName;
+  std::string BuiltinName;
----------------
Nit: I think we use the `overload` terminology rather than `generic`.


================
Comment at: clang/lib/Sema/SemaRVVLookup.cpp:359-371
+  if (!Record.MangledName)
+    MangledName = StringRef(Record.Name).split("_").first.str();
+  else
+    MangledName = Record.MangledName;
+  if (!SuffixStr.empty())
+    Name += "_" + SuffixStr.str();
+  if (!MangledSuffixStr.empty())
----------------
IIUC, above code initialize the BuiltinName, Name and MangledName same with RVVIntrinsic::RVVIntrinsic did, right?
If yes, I think we need to have some comment note that.


================
Comment at: clang/utils/TableGen/RISCVVEmitter.cpp:143
+  /// Emit all the information needed by SemaLookup.cpp.
+  void createSema(raw_ostream &o);
+
----------------
It will be good to have a description about why we need to have custom SemaLookup.


================
Comment at: clang/utils/TableGen/RISCVVEmitter.cpp:152
   void createRVVHeaders(raw_ostream &OS);
+  ///
+  unsigned GetSemaSignatureIndex(const SmallVector<std::string> &Signature);
----------------
missed comment?


================
Comment at: clang/utils/TableGen/RISCVVEmitter.cpp:163
 
-  // Emit the architecture preprocessor definitions. Return true when emits
-  // non-empty string.
-  bool emitExtDefStr(uint8_t Extensions, raw_ostream &o);
+#if 0
   // Slice Prototypes string into sub prototype string and process each sub
----------------
Forget to remove this code?


================
Comment at: clang/utils/TableGen/RISCVVEmitter.cpp:171
+  void EmitSemaRecords(raw_ostream &OS);
+  void ConstructSemaSignatureTable();
+  void EmitSemaSignatureTable(raw_ostream &OS);
----------------
Could we have comment for signature table. 
IIUC, it stores ProtoSeq, ProtoMaskSeq, SuffixProto and MangledSuffixProto information in each entry, and SemaRecord use the index to get the information from signature table, right?


================
Comment at: clang/utils/TableGen/RISCVVEmitter.cpp:853
+
+  for (auto SemaRecord : SemaRecords) {
+    InsertToSignatureSet(SemaRecord.ProtoSeq);
----------------
Use constant reference to access range-based loop?


================
Comment at: clang/utils/TableGen/RISCVVEmitter.cpp:860
+
+  for (auto Sig : Signatures) {
+    GetSemaSignatureIndex(Sig);
----------------
Don’t Use Braces on Simple Single-Statement Bodies of if/else/loop Statements.
Please check your code again.


================
Comment at: llvm/include/llvm/Support/RISCVVIntrinsicUtils.h:23
+
+enum RVVBasicType {
+  RVVBasicTypeUnknown = 0,
----------------
I think using typed enums is clearer because we would use TypeRangeMask to record supported basic types.
It should have the same type with TypeRangeMask.



================
Comment at: llvm/include/llvm/Support/RISCVVIntrinsicUtils.h:84
+  RVVTypeModifierLMUL1 = 1 << 6,
+  RVVTypeModifierMaskMax = RVVTypeModifierLMUL1,
+};
----------------
different naming rule and initialize way comparing to `RVVBasicTypeMaxOffset`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111617