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

[Daniel Sanders via Phabricator via llvm-commits]

dsanders added inline comments.


================
Comment at: lib/Target/AArch64/AArch64RegisterBankInfo.h:78
+                                      PartialMappingIdx LastAlias,
+                                      std::vector<PartialMappingIdx> Order) {
+    assert(Order.front() == FirstAlias && "First enumerator is not first");
----------------
ab wrote:
> 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> ?
That worked nicely. Thanks


https://reviews.llvm.org/D27976