[PATCH] D27336: [globalisel][aarch64] Replace magic numbers with corresponding enumerators in ValMappings. NFC
Quentin Colombet via llvm-commits
llvm-commits at lists.llvm.org
Mon Dec 5 09:20:35 PST 2016
> On Dec 5, 2016, at 3:43 AM, Daniel Sanders via Phabricator <reviews at reviews.llvm.org> wrote:
>
> dsanders added inline comments.
>
>
> ================
> Comment at: lib/Target/AArch64/AArch64GenRegisterBankInfo.def:41
> + LastFPR = FPR512,
> + PartialMappingIdx_Min = FirstGPR,
> };
> ----------------
> qcolombet wrote:
>> I'd use a shorter name: MinIdx or FirstIdx
> I was concerned about polluting the llvm::AArch64 namespace with generic-sounding identifiers. Looking at it again, this enum isn't in line with the coding standards (enumerators should have a common prefix) and fixing that would fix this problem. I think we should prefix them all with 'PMI_' to get:
> PMI_None,
> PMI_GPR32,
> ...
> PMI_LastFPR = PMI_FPR512,
> PMI_Min = PMI_FirstGPR
> Is that ok with you?
Definitely.
Thanks
>
>
> ================
> Comment at: lib/Target/AArch64/AArch64GenRegisterBankInfo.def:85
> // 0: GPR 32-bit value. <-- This must match First3OpsIdx.
> - {&PartMappings[0], 1}, {&PartMappings[0], 1}, {&PartMappings[0], 1},
> + {&PartMappings[GPR32 - PartialMappingIdx_Min], 1}, {&PartMappings[GPR32 - PartialMappingIdx_Min], 1}, {&PartMappings[GPR32 - PartialMappingIdx_Min], 1},
> // 3: GPR 64-bit value.
> ----------------
> qcolombet wrote:
>> Could you clang-format those lines?
> Ok.
>
>
> https://reviews.llvm.org/D27336
>
>
>
More information about the llvm-commits
mailing list