[PATCH] D27338: [globalisel] Tablegen-erate current Register Bank Information

[Diana Picus via Phabricator via llvm-commits]

rovka added a comment.

Just a few nitpicks, other than that LGTM (but you should probably wait for someone more experienced to approve).



================
Comment at: lib/Target/AArch64/AArch64RegisterBankInfo.h:19
 
+#define TBLGEN_REGISTERBANKINFO_H
+#include "AArch64GenRegisterBank.inc"
----------------
I think it's nicer to use a macro that describes what we're trying to include rather than where we're including it, e.g. GET_REGBANK_DECLS or something of the sort. This also seems to be the style used in GenRegisterInfo.inc, GenSubtargetInfo.inc etc.


================
Comment at: utils/TableGen/RegisterBankEmitter.cpp:202
+///   is a member.
+static void visitSelfAndSubclassesAndClassesWithSubRegs(
+    CodeGenRegBank &RegisterClassHierarchy, const RegisterBank &Bank,
----------------
How about visitRegisterBankClasses? The comment is clear enough on what something like that would mean :)


================
Comment at: utils/TableGen/RegisterBankEmitter.cpp:233
+    StringRef Enumeration, StringRef EnumeratorPrefix) {
+  OS << "    assert(";
+  for (RegisterBank::PartialMappingsTy::const_iterator I = Begin;
----------------
I think it would be cleaner to generate a comment instead of a moot assertion if Begin == End or Begin + 1 == End (and then you can also remove the I != End check in the loop).


================
Comment at: utils/TableGen/RegisterBankEmitter.cpp:304
+    } else {
+      OS << "    assert(" << PartialMappingEnumerationName.str() << "::PMI_"
+         << Bank.getName() << Bank.getRCWithSmallestRegsSize()->SpillSize
----------------
Are the calls to .str() really necessary?


================
Comment at: utils/TableGen/RegisterBankEmitter.cpp:309
+         << "\"" << Bank.getName()
+         << Bank.getRCWithSmallestRegsSize()->SpillSize << " not last in the "
+         << Bank.getName() << " list\");\n";
----------------
" not first "


https://reviews.llvm.org/D27338