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

[Carl Ritson via Phabricator via llvm-commits]

critson marked 8 inline comments as done.
critson added inline comments.


================
Comment at: llvm/utils/TableGen/RegisterInfoEmitter.cpp:1612
+    if (!BaseClasses.empty()) {
+      assert(BaseClasses.size() < (UINT8_MAX - 1) && "Too many base register classes");
+
----------------
foad wrote:
> `< UINT8_MAX` should be enough.
Not if we use first element for nullptr (in revised version).


================
Comment at: llvm/utils/TableGen/RegisterInfoEmitter.cpp:1622
+          else
+            return LHSOrder < RHSOrder;
+        }
----------------
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.


================
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) {
----------------
arsenm wrote:
> foad wrote:
> > Why `+ 1`?
> Can do this directly in constructor
`+ 1` is for NoRegister which is not part of Regs.  I have added a comment to document this.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139616