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

[Fraser Cormack via Phabricator via llvm-commits]

frasercrmck 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.
----------------
craig.topper wrote:
> This set unset but you're using empty string instead of '?'
As in a comment below I think `<string name = NAME>` and `field string Name = name` is what I want here.


================
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.
----------------
craig.topper wrote:
> Would StringRef be better?
Yeah, I'll do that. Ta


================
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");
----------------
craig.topper wrote:
> `EnumClass` is unused in release builds
Ah yes, thanks! I'll just `assert(Records.getClass("Enum"))` then


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


================
Comment at: llvm/utils/TableGen/EnumEmitter.cpp:77
+static StringRef getEnumMemberName(const Record *EM) {
+  if (auto EnumMemName = EM->getValueAsOptionalString("Name"))
+    if (!EnumMemName->empty())
----------------
craig.topper wrote:
> Why getValueAsOptionalString and not getValueAsString? Are you intending to allow Unset as well as empty?
I was kind of hoping `EnumMember` could have both a default of `NAME` and a template parameter for a common situation where it might need renaming, without relying on an extra derived class. I think `class EnumMember<string name = NAME>` is the right way to do that? In which case, no, this should never be unset - or empty, really? Should I assert that it isn't empty, then?


================
Comment at: llvm/utils/TableGen/EnumEmitter.cpp:117
+  // Emit the enum definition itself
+  std::string EnumGuard = "GET_" + EnumName.str() + "_ENUM";
+  OS << "#ifdef " << EnumGuard << "\n";
----------------
myhsu wrote:
> I wonder if it will be better to use Twine here?
Done


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


================
Comment at: llvm/utils/TableGen/EnumEmitter.cpp:141
+  // Emit the print (or 'as string') function definition
+  if (!Enum->isValueUnset("PrintMethod")) {
+    if (HasDuplicateVals)
----------------
myhsu wrote:
> can we simplify this using Record::getValueAsOptionalString?
Much better, thank you!


================
Comment at: llvm/utils/TableGen/EnumEmitter.cpp:155
+    for (const auto *M : Members) {
+      OS.indent(2) << "case " << Twine(EnumName + "::" + getEnumMemberName(M))
+                   << ":\n";
----------------
craig.topper wrote:
> Why do we need a Twine in the middle here?
We don't! Not sure what was happening there.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122832