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

[Quentin Colombet via Phabricator via llvm-commits]

qcolombet added a comment.

Hi Daniel,

Mostly looks good to me, I would however suggest two changes:

1. Drop the partial mapping support.
2. Add all the register classes covered by a RegisterBank statically.

For #1, I think that part should belong to something related to instruction pattern emitter.

For #2, the call to addRegBankCoverage still does a bunch of stuff at runtime whereas all doable at compile time. Indeed, I expect the tablegen backend to just populate the bit vector at compile time. Ultimately, I would like to change the representation of the covered register class into an array of uint32_t, so that we just get rid of the complexity of the BitVector. You can have a look at SubClassMask in TargetRegisterInfo and how we generate it to have an idea of what I am talking about.

Cheers,
-Quentin



================
Comment at: include/llvm/Target/GlobalISel/RegisterBank.td:19
+  //        register classes but currently don't.
+  list<int> ExcludePartialMappings = [];
+}
----------------
This seems gross.
I would rather patch the manual mapping than having to add this distinction.
Namely, right now we use clever tricks to access the element in the array, but I could totally see an enum or an array doing that mapping.
The reason why I did this was because it was the fastest thing to do :P.

The bottom line is, let us clean that up!


================
Comment at: utils/TableGen/RegisterBankEmitter.cpp:202
+///   is a member.
+static void visitSelfAndSubclassesAndClassesWithSubRegs(
+    CodeGenRegBank &RegisterClassHierarchy, const RegisterBank &Bank,
----------------
dsanders wrote:
> 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()
Right now, this method is used only once. Are there any plan to reuse it with a different functor?
If not, let us name it addSubRegisterClasses.


================
Comment at: utils/TableGen/RegisterBankEmitter.cpp:38
+  /// handle partial mappings yet.
+  PartialMappingsTy PartialMappings;
+
----------------
Partial mappings are more a consequence of instruction patterns than register bank. I wouldn't expect this to happen here.


https://reviews.llvm.org/D27338