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

[Daniel Sanders via llvm-commits]

> On 5 Dec 2016, at 17:27, Quentin Colombet via llvm-commits <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.

>>> 
>>> 
>>> 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/71911d7c/attachment.html>