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

[Kito Cheng via Phabricator via llvm-commits]

kito-cheng added inline comments.


================
Comment at: clang/lib/Sema/SemaRVVLookup.cpp:91
+struct RVVIntrinsicDef {
+  std::string Name;
+  std::string GenericName;
----------------
khchen wrote:
> why do we need to declare Name as std::string here but RVVIntrinsicRecord use `const char*`?
`RVVIntrinsicRecord::Name` is raw name of a intrinsic, `RVVIntrinsicDef::Name` is expanded with type infos, e.g. `RVVIntrinsicRecord::Name` is `vadd` and  `RVVIntrinsicDef::Name` is `vadd_vv_i32m1`.


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


================
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())
----------------
khchen wrote:
> 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.
More comment added.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111617