[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:1644
+      OS << " };\n\n";
+      OS << "  if (Reg >= sizeof(Mapping))\n    return nullptr;\n";
+      OS << "  return BaseClasses[Mapping[Reg]];\n";
----------------
Just assert `Reg < sizeof(Mapping)`. Needs a small fix in IsCopyFromSGPR in D139422.


================
Comment at: llvm/utils/TableGen/RegisterInfoEmitter.cpp:1612
+    if (!BaseClasses.empty()) {
+      assert(BaseClasses.size() < (UINT8_MAX - 1) && "Too many base register classes");
+
----------------
critson wrote:
> foad wrote:
> > `< UINT8_MAX` should be enough.
> Not if we use first element for nullptr (in revised version).
It would work fine to emit `BaseClasses[256]`, so `BaseClasses.size()` can be 255, but you're restricting it to a max value of 253 here, so it's an off-by-2 error.


================
Comment at: llvm/utils/TableGen/RegisterInfoEmitter.cpp:1622
+          else
+            return LHSOrder < RHSOrder;
+        }
----------------
critson wrote:
> foad wrote:
> > This is `return std::tie(LHS->getBaseClassOrder(), LHS->EnumValue) < std::tie(RHS->getBaseClassOrder(), RHS->EnumValue)`
> `std::tie` cannot handle `std::optional` argument it seems, so I have done this with `std::pair` instead.
Comparing the `std::optional`s themselves is a bit silly when you have already checked that they both have a value. I would suggest just adding a `*`: `std::tie(*LHS->getBaseClassOrder(), LHS->EnumValue) ...`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139616