[PATCH] D139616: [TableGen] Emit table mapping physical registers to base classes

[Jay Foad via Phabricator via llvm-commits]

foad added inline comments.


================
Comment at: llvm/utils/TableGen/RegisterInfoEmitter.cpp:1202
+  if (llvm::any_of(RegisterClasses, [](const auto &RC) {
+                   return RC.getBaseClassOrder() != std::nullopt; })) {
+    OS << "  const TargetRegisterClass *getPhysRegBaseClass(MCRegister Reg) const override;\n";
----------------
Comparing with nullopt is weird. Can't you rely on implicit conversion to bool?


================
Comment at: llvm/utils/TableGen/RegisterInfoEmitter.cpp:1608
+    for (const auto &RC : RegisterClasses) {
+      if (auto Order = RC.getBaseClassOrder())
+        BaseClasses.push_back(&RC);
----------------
You're not using Order here.


================
Comment at: llvm/utils/TableGen/RegisterInfoEmitter.cpp:1612
+    if (!BaseClasses.empty()) {
+      assert(BaseClasses.size() < (UINT8_MAX - 1) && "Too many base register classes");
+
----------------
`< UINT8_MAX` should be enough.


================
Comment at: llvm/utils/TableGen/RegisterInfoEmitter.cpp:1622
+          else
+            return LHSOrder < RHSOrder;
+        }
----------------
This is `return std::tie(LHS->getBaseClassOrder(), LHS->EnumValue) < std::tie(RHS->getBaseClassOrder(), RHS->EnumValue)`


================
Comment at: llvm/utils/TableGen/RegisterInfoEmitter.cpp:1629
+      std::vector<uint8_t> Mapping;
+      Mapping.resize(Regs.size() + 1, 0);
+      for (int RCIdx = BaseClasses.size() - 1; RCIdx >= 0; --RCIdx) {
----------------
Why `+ 1`?


================
Comment at: llvm/utils/TableGen/RegisterInfoEmitter.cpp:1649
+      OS << "  if (Reg >= sizeof(Mapping))\n    return nullptr;\n";
+      OS << "  if (!Mapping[Reg])\n    return nullptr;\n";
+      OS << "  return BaseClasses[Mapping[Reg] - 1];\n";
----------------
Put nullptr as the first entry in the table to avoid this `if`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139616