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

[Daniel Sanders via Phabricator via llvm-commits]

dsanders marked 2 inline comments as done.
dsanders added a comment.

Thanks. I've updated the patch.



================
Comment at: utils/TableGen/RegisterBankEmitter.cpp:202
+///   is a member.
+static void visitSelfAndSubclassesAndClassesWithSubRegs(
+    CodeGenRegBank &RegisterClassHierarchy, const RegisterBank &Bank,
----------------
rovka wrote:
> How about visitRegisterBankClasses? The comment is clear enough on what something like that would mean :)
As you can tell, I struggled to name this one :-). It doesn't really fit with 'overlapping', 'subclasses', 'subsets', etc.

I'll go with visitRegisterBankClasses()


================
Comment at: utils/TableGen/RegisterBankEmitter.cpp:233
+    StringRef Enumeration, StringRef EnumeratorPrefix) {
+  OS << "    assert(";
+  for (RegisterBank::PartialMappingsTy::const_iterator I = Begin;
----------------
rovka wrote:
> 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).
The caller filters out the Begin == End case on line 303 (since Bank.hasPartialMappings() is a test for Begin != End). I think it's good to explicitly assert that here which leads to the same simplification you suggest.

Is that ok?


================
Comment at: utils/TableGen/RegisterBankEmitter.cpp:304
+    } else {
+      OS << "    assert(" << PartialMappingEnumerationName.str() << "::PMI_"
+         << Bank.getName() << Bank.getRCWithSmallestRegsSize()->SpillSize
----------------
rovka wrote:
> Are the calls to .str() really necessary?
I thought I needed them for type-conversion but apparently not. I've gone through the other .str()'s too.


https://reviews.llvm.org/D27338