[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");
----------------
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?


https://reviews.llvm.org/D27976