[PATCH] D27976: [aarch64][globalisel] Move data into <Target>GenRegisterBankInfo. NFC.
Daniel Sanders via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Mon Jan 9 03:26:37 PST 2017
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
More information about the llvm-commits
mailing list