[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