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

[Carl Ritson via Phabricator via llvm-commits]

critson marked 3 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:
> 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.
OK, yes, because of the comparison being less-than.


================
Comment at: llvm/utils/TableGen/RegisterInfoEmitter.cpp:1622
+          else
+            return LHSOrder < RHSOrder;
+        }
----------------
foad wrote:
> 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) ...`
I agree the optionals can be dereferenced, but we still cannot use `std::tie` as they resulting value is not an l-value.  I really don't think it matters to use `std::pair` here (or `std::tuple`), this code is executed once during tablegen on a maximum of 254 values.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139616