[PATCH] D27976: [aarch64][globalisel] Move data into <Target>GenRegisterBankInfo. NFC.

[Ahmed Bougacha via Phabricator via llvm-commits]

ab accepted this revision.
ab added a comment.

This LGTM, but I'll let Quentin have final say.



================
Comment at: lib/Target/AArch64/AArch64RegisterBankInfo.h:32
 
+class AArch64GenRegisterBankInfo : public RegisterBankInfo {
+private:
----------------
qcolombet wrote:
> dsanders wrote:
> > qcolombet wrote:
> > > FWIW, I believe that generating that part could be part of the tablegen switch. I indeed don't think it makes much to have a XXXGen manually written. I.e., could be squashed with the tablegen switch.
> > > 
> > > But I am fine with the intermediate step if it does not last long.
> > It could be part of the tablegen switch. The reason it isn't is that Ahmed suggested adding the class first and then having a tablegen patch make tablegen emit that same implementation.
> Up to you.
> I would say that either way is fine as long as XXXGenRegisterBankInfo gets generated by TableGen shortly.
> It could be part of the tablegen switch. The reason it isn't is that Ahmed suggested adding the class first and then having a tablegen patch make tablegen emit that same implementation.

Sorry if I was unclear: I think the tablegen patch should be NFC (the main issue being PartialMapping), but not necessarily that it generates exactly the same code as existed before.  So, I'd also say squash this with the tablegen switch, but it's not a big deal either way.


================
Comment at: lib/Target/AArch64/AArch64RegisterBankInfo.h:78
+                                      PartialMappingIdx LastAlias,
+                                      std::vector<PartialMappingIdx> Order) {
+    assert(Order.front() == FirstAlias && "First enumerator is not first");
----------------
dsanders wrote:
> dsanders wrote:
> > qcolombet wrote:
> > > qcolombet wrote:
> > > > I would prefer that we call this checkXXX and return a bool.
> > > > The assert is then part of the caller, i.e., assert(checkXXX).
> > > const std::vector<...> &
> > > I would prefer that we call this checkXXX and return a bool.
> > > The assert is then part of the caller, i.e., assert(checkXXX).
> > 
> > Ok.
> > 
> > > const std::vector<...> &
> > 
> > Well spotted
> I haven't made the change to a `const std::vector<...> &` because the arguments are initializer_lists and cannot be bound to the reference type. I could fix this but it makes the caller side a bit uglier:
>   std::vector<PartialMappingIdx> Temp = {PMI_GPR32, PMI_GPR64};
>   checkPartialMappingIdx(..., Temp);
> and only saves us a small constant amount of time in debug builds.
> 
> Do you think I should leave it as-is or go with the uglier code?
What about ArrayRef<PartialMappingIdx> ?


https://reviews.llvm.org/D27976