[PATCH] D122832: [TableGen] Add a backend to help with target enums

[Craig Topper via Phabricator via llvm-commits]

craig.topper added inline comments.


================
Comment at: llvm/include/llvm/Target/Target.td:1776
+
+  // If given, this is the name given to the enumeration member. If unset,
+  // the name of the Record definition is used.
----------------
This set unset but you're using empty string instead of '?'


================
Comment at: llvm/include/llvm/Target/Target.td:1803
+// Generates:
+//     enum MuEnum {
+//       A,
----------------
MuEnum -> MyEnum?


================
Comment at: llvm/include/llvm/Target/Target.td:1823
+  // The name of the method used to get the String value of enumeration
+  // values. The generated method returns const char *, and switches over all
+  // enum members with an llvm_unreachable on the default case.
----------------
Would StringRef be better?


================
Comment at: llvm/utils/TableGen/EnumEmitter.cpp:25
+/// Generates:
+///     enum MuEnum {
+///       A,
----------------
MuEnum -> MyEnum?


================
Comment at: llvm/utils/TableGen/EnumEmitter.cpp:63
+void EnumEmitter::run(raw_ostream &OS) const {
+  auto *EnumClass = Records.getClass("Enum");
+  assert(EnumClass && "Enum class definition missing");
----------------
`EnumClass` is unused in release builds


================
Comment at: llvm/utils/TableGen/EnumEmitter.cpp:67
+  std::vector<const Record *> Enums;
+  for (const auto *D : Records.getAllDerivedDefinitions("Enum"))
+    Enums.push_back(D);
----------------
Doesn't getAllDerivedDefinitions return a std::vector? Can we just assign it to Enums instead of copying every value.


================
Comment at: llvm/utils/TableGen/EnumEmitter.cpp:77
+static StringRef getEnumMemberName(const Record *EM) {
+  if (auto EnumMemName = EM->getValueAsOptionalString("Name"))
+    if (!EnumMemName->empty())
----------------
Why getValueAsOptionalString and not getValueAsString? Are you intending to allow Unset as well as empty?


================
Comment at: llvm/utils/TableGen/EnumEmitter.cpp:108
+  bool HasDuplicateVals = false;
+  // Keep track of enumerator values and names as duplicates would cause C++
+  // miscompilations. These would be caught later by the C++ compiler, but it's
----------------
command after "names" I think.


================
Comment at: llvm/utils/TableGen/EnumEmitter.cpp:120
+  OS << "enum " << (IsClass ? "class " : "") << EnumName
+     << (UnderlyingTy ? " : " + *UnderlyingTy : "") << " {\n";
+  for (const auto &P : enumerate(Members)) {
----------------
Maybe just a regular `if` statement for UnderlyingType being set instead of the conditional operator and an empty string.


================
Comment at: llvm/utils/TableGen/EnumEmitter.cpp:155
+    for (const auto *M : Members) {
+      OS.indent(2) << "case " << Twine(EnumName + "::" + getEnumMemberName(M))
+                   << ":\n";
----------------
Why do we need a Twine in the middle here?


================
Comment at: llvm/utils/TableGen/EnumEmitter.cpp:179
+      OS.indent(6) << ".Case(\"" << getEnumMemberStr(M) << "\", "
+                   << Twine(EnumName + "::" + getEnumMemberName(M)) << ")\n";
+    }
----------------
Same question about the Twine here


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122832