[PATCH] D27337: [globalisel][aarch64] Fix unintended assumptions about PartialMappingIdx. NFC.

[Quentin Colombet via llvm-commits]

> On Dec 5, 2016, at 11:19 AM, Daniel Sanders <daniel_l_sanders at apple.com> wrote:
> 
>> 
>> On 5 Dec 2016, at 17:27, Quentin Colombet via llvm-commits <llvm-commits at lists.llvm.org <mailto:llvm-commits at lists.llvm.org>> wrote:
>> 
>>> 
>>> On Dec 5, 2016, at 9:22 AM, Quentin Colombet via llvm-commits <llvm-commits at lists.llvm.org <mailto:llvm-commits at lists.llvm.org>> wrote:
>>> 
>>>> 
>>>> On Dec 5, 2016, at 3:54 AM, Daniel Sanders via Phabricator <reviews at reviews.llvm.org <mailto:reviews at reviews.llvm.org>> wrote:
>>>> 
>>>> dsanders added inline comments.
>>>> 
>>>> 
>>>> ================
>>>> Comment at: lib/Target/AArch64/AArch64RegisterBankInfo.cpp:118
>>>>    const PartialMapping &Map =                                                \
>>>> -        AArch64::PartMappings[AArch64::PartialMappingIdx::Idx];                \
>>>> +        AArch64::PartMappings[AArch64::Idx - AArch64::PartialMappingIdx_Min];  \
>>>>    (void) Map;                                                                \
>>>> ----------------
>>>> qcolombet wrote:
>>>>> I'd keep `::PartialMappingIdx::`, I like being explicit about the type we use (and that shouldn't be redundant for the _Min part when you'd use a shorter name, see the other review).
>>>> Ok. Would you like me to make the explicit namespace mandatory by changing the type to an 'enum class'? This would also fix the prefix issue I mentioned on the other review.
>>> 
>>> No, following the prefix rule like you did in the other review is enough?
>> 
>> Scratch the question mark x)
> 
> Ok, I'll go with the 'PMI_' prefix.
> 
>>> 
>>>> 
>>>> 
>>>> ================
>>>> Comment at: lib/Target/AArch64/AArch64RegisterBankInfo.cpp:135
>>>>  do {                                                                         \
>>>> -    AArch64::PartialMappingIdx PartialMapBaseIdx =                             \
>>>> -        AArch64::PartialMappingIdx::RBName##Size;                              \
>>>> +    unsigned PartialMapBaseIdx =                                               \
>>>> +        AArch64::RBName##Size - AArch64::PartialMappingIdx_Min;                \
>>>> ----------------
>>>> qcolombet wrote:
>>>>> Could we keep the PartialMappingIdx type instead of unsigned?
>>>> The compiler warns about using PartialMappingIdx because it's not a PartialMappingIdx after the subtraction. The warning could be silenced with a cast like so:
>>>> PartialMappingIdx PartialMapBaseIdx = (PartialMappingIdx)(AArch64::PartialMappingIdx::RBName##Size - AArch64::PartialMappingIdx::PartialMappingIdx_Min);
>>>> but this is still misleading because the values don't match up with the enumerators.
>>> 
>>> What to you mean they do not match up with the enumerators?
>>> The value should be within the enumerators at the end of the subtraction.
> 
> I mean that when PartialMapBaseIdx is GPR32 it actually refers to the GPR64 element of PartMappings (GPR64 - Min == 2 - 1 == 1 == GPR32). It's only an issue because Min is non-zero which is needed to prove we caught all the missing subtractions. We can put Min back to 0 at the end of the patch series (including a couple I'll post tomorrow) if we want.

Got it.
Let us keep it the way you have it now.

> 
>>>> 
>>>> 
>>>> https://reviews.llvm.org/D27337 <https://reviews.llvm.org/D27337>
>>>> 
>>>> 
>>>> 
>>> 
>>> _______________________________________________
>>> llvm-commits mailing list
>>> llvm-commits at lists.llvm.org <mailto:llvm-commits at lists.llvm.org>
>>> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits <http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits>
>> _______________________________________________
>> llvm-commits mailing list
>> llvm-commits at lists.llvm.org <mailto:llvm-commits at lists.llvm.org>
>> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits <http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20161205/8622b6dc/attachment.html>