[PATCH] D27338: [globalisel] Tablegen-erate current Register Bank Information
Quentin Colombet via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Wed Dec 14 17:46:54 PST 2016
qcolombet added a comment.
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.
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,
> 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.
More information about the llvm-commits