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

[Craig Topper via Phabricator via cfe-commits]

craig.topper added inline comments.


================
Comment at: clang/utils/TableGen/RISCVVEmitter.cpp:70
+  // passing to the BUILTIN() macro in Builtins.def.
+  const std::string &builtin_str() const { return BuiltinStr; }
+
----------------
These method names should use CamelCase and start with "get"


================
Comment at: clang/utils/TableGen/RISCVVEmitter.cpp:102
+  // Compute and record a string for legal type.
+  void compute_builtin_str();
+  // Compute and record a builtin RVV vector type string.
----------------
These should use CamelCase per llvm coding style.

Might be better named init*Str instead of compute. compute makes me think they are going to return something, but that might just be me.


================
Comment at: clang/utils/TableGen/RISCVVEmitter.cpp:267
+RVVType::RVVType(BasicType BT, int Log2LMUL, StringRef prototype)
+    : BT(BT), LMUL(LMULType(Log2LMUL)), IsFloat(false), IsBool(false),
+      IsSigned(true), IsImmediate(false), IsVoid(false), IsConstant(false),
----------------
You can initialize the bools to false with " = false" where they are declared in the class body then you don't need to mention them all here. Similar for Scale and ElementBitWidth.


================
Comment at: clang/utils/TableGen/RISCVVEmitter.cpp:270
+      IsPointer(false), IsSize_t(false), IsPtrdiff_t(false),
+      ElementBitwidth(~0U), Scale(0) {
+  applyBasicType();
----------------
Why is ElementBitwidth default ~0. Wouldn't 0 also be an invalid value?


================
Comment at: clang/utils/TableGen/RISCVVEmitter.cpp:333
+    return;
+  } else if (IsSize_t) {
+    S = "z";
----------------
You drop the else since the if above returned.


================
Comment at: clang/utils/TableGen/RISCVVEmitter.cpp:618
+  if (NewMangledName.empty())
+    MangledName = Twine(NewName.split("_").first).str();
+  else
----------------
I don't think we need to go through Twine here. We should be able to call str() directly on first.


================
Comment at: clang/utils/TableGen/RISCVVEmitter.cpp:621
+    MangledName = NewMangledName.str();
+  if (Suffix.size())
+    Name += "_" + Suffix.str();
----------------
!Suffix.empty()


================
Comment at: clang/utils/TableGen/RISCVVEmitter.cpp:640
+  InputTypes.assign(OutInTypes.begin() + 1, OutInTypes.end());
+  for (unsigned i = 0; i < InputTypes.size(); ++i)
+    CTypeOrder.push_back(i);
----------------
CTypeOrder.resize(InputTypes.size());
std::iota(CTypeOrder.begin(), CTypeOrder.end(), 0);


================
Comment at: clang/utils/TableGen/RISCVVEmitter.cpp:710
+void RVVIntrinsic::emitMangledFuncDef(raw_ostream &OS) const {
+  OS << Twine(OutputType->type_str() + Twine(" ")).str();
+  OS << getMangledName();
----------------
Can't we just print OutputType->type_str() and " " to OS separately? We shouldn't need to concat them into a Twine first.


================
Comment at: clang/utils/TableGen/RISCVVEmitter.cpp:714
+  // Emit function arguments
+  if (CTypeOrder.size() > 1) {
+    OS << InputTypes[CTypeOrder[0]]->type_str() + " op0";
----------------
Why is this > 1 and not >= 1 or !CTypeOrder.empty()?


================
Comment at: clang/utils/TableGen/RISCVVEmitter.cpp:720
+  OS << "){\n";
+  OS << "  return " + getName() + "(";
+  // Emit parameter variables
----------------
Replace the + operators with <<


================
Comment at: clang/utils/TableGen/RISCVVEmitter.cpp:722
+  // Emit parameter variables
+  if (CTypeOrder.size() > 1) {
+    OS << "op0";
----------------
Same here, why is this >1 and not >=1 or !CTypeOrder.empty()?


================
Comment at: clang/utils/TableGen/RISCVVEmitter.cpp:749
+
+  OS << "#ifndef _RISCV_VECTOR_H\n";
+  OS << "#define _RISCV_VECTOR_H\n\n";
----------------
Looks like other headers use 2 underscores at the beginning of their include guard.


================
Comment at: clang/utils/TableGen/RISCVVEmitter.cpp:757
+  OS << "#error \"Vector intrinsics require the vector extension.\"\n";
+  OS << "#else\n\n";
+
----------------
Can we just #endif here instead of the #else? If the error is emitted the preprocessor should stop and not process the rest of the file. Then we don't need to close it at the bottom of the file.


================
Comment at: clang/utils/TableGen/RISCVVEmitter.cpp:767
+  // Dump RVV boolean types.
+  auto dumpType = [&](auto T) {
+    OS << "typedef " << T->clang_builtin_str() << " " << T->type_str() << ";\n";
----------------
I'd recommend calling this printType.  dump made me think it was printing for debug like the dump() functions found in many LLVM classes.


================
Comment at: clang/utils/TableGen/RISCVVEmitter.cpp:799
+  // D implies F
+  OS << "#if defined(__riscv_f) || defined(__riscv_d)\n";
+  for (int Log2LMUL : Log2LMULs) {
----------------
I think we only need to check __riscv_f here?


================
Comment at: clang/utils/TableGen/RISCVVEmitter.cpp:836
+      return;
+    OS << StringRef(
+        "static inline __attribute__((__always_inline__, __nodebug__, "
----------------
I don't think this StringRef constructor call is needed.


================
Comment at: clang/utils/TableGen/RISCVVEmitter.cpp:849
+  OS << "#define RISCVV_BUILTIN(ID, TYPE, ATTRS) BUILTIN(ID, TYPE, ATTRS)\n";
+  OS << "#endif\n";
+  for (auto &Def : Defs) {
----------------
We probably want 2 newlines on the end of this so there will be a blank line before the first RISCVV_BUILTIN.


================
Comment at: clang/utils/TableGen/RISCVVEmitter.cpp:873
+  // Dump switch body when the ir name changes from previous iteration.
+  RVVIntrinsic *PrevDef = Defs.begin()->get();
+  for (auto &Def : Defs) {
----------------
Can we remember the PrevIRName StringRef instead?


================
Comment at: clang/utils/TableGen/RISCVVEmitter.cpp:883
+  }
+  (*Defs.rbegin())->emitCodeGenSwitchBody(OS);
+  OS << "break;\n";
----------------
Defs.back()->emitCodeGenSwitchBody(OS);


================
Comment at: clang/utils/TableGen/RISCVVEmitter.cpp:1027
+    return false;
+  bool NeedOR = false;
+  OS << "#if";
----------------
I think you can use ListSeparator for this. It keeps track of the separator string and whether the first item has been printed. It's most often used with loops, but it should work here. I think there are many examples uses in llvm/utils/TableGen


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95016