[PATCH] D95016: [Clang][RISCV] Add custom TableGen backend for riscv-vector intrinsics.

[Craig Topper via Phabricator via llvm-commits]

craig.topper added inline comments.


================
Comment at: clang/utils/TableGen/RISCVVEmitter.cpp:70
+  // passing to the BUILTIN() macro in Builtins.def.
+  std::string builtin_str() const { return BuiltinStr; }
+
----------------
Return a const std::string & or a StringRef.


================
Comment at: clang/utils/TableGen/RISCVVEmitter.cpp:85
+  bool isScalar() const {
+    return (Vscale.hasValue() && Vscale.getValue() == 0);
+  }
----------------
Drop parentheses


================
Comment at: clang/utils/TableGen/RISCVVEmitter.cpp:154
+  // Return the architecture preprocessor definitions.
+  static SmallVector<std::string> getExtStrings(uint8_t Extensions);
+
----------------
Does this need to be in Intrinsic? Can it just be in the emitter class?


================
Comment at: clang/utils/TableGen/RISCVVEmitter.cpp:984
+  StringRef MangledSuffix = R->getValueAsString("MangledSuffix");
+  std::string Prototypes = R->getValueAsString("Prototype").data();
+  StringRef TypeRange = R->getValueAsString("TypeRange");
----------------
Use str() not data().

But I'm not sure why it can't just a be StringRef?


================
Comment at: clang/utils/TableGen/RISCVVEmitter.cpp:997
+
+  // Parse prototype and create a list of primitve type with transformers
+  // (operand) in ProtoSeq. ProtoSeq[0] is output operand.
----------------
primitive*


================
Comment at: clang/utils/TableGen/RISCVVEmitter.cpp:1070
+  // search first
+  if (LegalTypes.count(Idx)) {
+    return Optional<RVVTypePtr>(LegalTypes[Idx]);
----------------
Use LegalTypes.find() so you don't have to two look ups.


================
Comment at: clang/utils/TableGen/RISCVVEmitter.cpp:1076
+  // compute type and record the result
+  auto T = std::make_shared<RVVType>(BT, LMUL, Proto);
+  if (T->isValid()) {
----------------
Does this need to be shared_ptr? Can we just arrange for LegalTypes to own the types and give every one else a pointer? LegalTypes would just need to outlive the references to the types.


================
Comment at: clang/utils/TableGen/RISCVVEmitter.cpp:1102
+    if (ExtStrings.size()) {
+      std::string ArchMacro = std::accumulate(
+          ExtStrings.begin() + 1, ExtStrings.end(), "(" + ExtStrings[0] + ")",
----------------
Can we just stream this out to OS instead of accumulating a string before streaming?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95016