[PATCH] D39823: GlobalISel: Enable the legalization of G_MERGE_VALUES and G_UNMERGE_VALUES

[Volkan Keles via llvm-commits]

> On Nov 17, 2017, at 10:59 AM, Daniel Sanders <daniel_l_sanders at apple.com> wrote:
> 
> 
> 
>> On 17 Nov 2017, at 10:15, Quentin Colombet <qcolombet at apple.com <mailto:qcolombet at apple.com>> wrote:
>> 
>> 
>> 
>>> On Nov 14, 2017, at 4:43 PM, Volkan Keles <vkeles at apple.com <mailto:vkeles at apple.com>> wrote:
>>> 
>>> 
>>> 
>>>> On Nov 13, 2017, at 4:32 PM, Quentin Colombet <qcolombet at apple.com <mailto:qcolombet at apple.com>> wrote:
>>>> 
>>>> 
>>>> 
>>>>> On Nov 13, 2017, at 8:51 AM, Kristof Beyls via Phabricator <reviews at reviews.llvm.org <mailto:reviews at reviews.llvm.org>> wrote:
>>>>> 
>>>>> kristof.beyls added inline comments.
>>>>> 
>>>>> 
>>>>> ================
>>>>> Comment at: lib/CodeGen/GlobalISel/LegalizerInfo.cpp:194-201
>>>>> +    unsigned Op = MI.getOperand(i).getReg();
>>>>> +    // G_UNMERGE_VALUES has variable number of operands, but there is only
>>>>> +    // one source type and one destination type as all destinations must be the
>>>>> +    // same type. So, get the last operand if TypeIdx == 1.
>>>>> +    if (MI.getOpcode() == TargetOpcode::G_UNMERGE_VALUES && TypeIdx == 1) {
>>>>> +      Op = MI.getOperand(MI.getNumOperands() - 1).getReg();
>>>>> +    }
>>>>> ----------------
>>>>> To make this easier to read, I think it may make sense to factor this out into a separate function, e.g. with a name like "getTyFromTypeIdx(MI, TypeIdx)". I'm not sure on the name, but doing it this way would make this code a lot easier to understand, I think.
>>>>> 
>>>>> 
>>>>> ================
>>>>> Comment at: lib/Target/AArch64/AArch64LegalizerInfo.cpp:359
>>>>> +  for (const auto &Ty :
>>>>> +       {s32, s64, s96, s128, s192, v2s32, v3s32, v4s32, v2s64, v4s64, v6s64}) {
>>>>> +    setAction({G_MERGE_VALUES, Ty}, Legal);
>>>>> ----------------
>>>>> I haven't thought this through at all, but I'm a bit surprised v8s64 also isn't legal if v6s64 is?
>>>>> Similarly for S256 if s192 is legal?
>>>> 
>>>> FWIW, I believe we should just specify the size (instead of full types) for operations that deal only with bit manipulation (merge_value, phi, bitwise operation, ld/st).
>>>> 
>>> 
>>> I think it’s good to legalize these operations based on full types because legalization actions might be different, especially for vector types.
>> 
>> Given that those instructions only deal with bag of bits, I don’t see why the full types are relevant.
>> What am I missing?
> 
> For some targets, an s64 load and an v2s32 load produce different bitpatterns in the register. For example, on big-endian Mips (and big-endian AArch64 I think) the elements are big-endian but element zero is still at offset 0 in memory and at bit 0 in the register:
> Memory: 00 11 22 33 44 55 66 77
> s64 load: 0x0011223344556677
> v2s32 load: 0x4455667700112233
> 
> Similarly, merge_values would need to behave differently depending for vectors and scalars:
> s64 = G_MERGE_VALUES s32 0x00112233, s32 0x44556677 ; 0x0011223344556677
> v2s32 = G_MERGE_VALUES s32 0x00112233, s32 0x44556677 ; 0x4455667700112233

Another use case might be handling vector types. For example, <2 x s8> might not be legal and we should be able to set action for that type like MoreElements. Also, if we are going to add more opcodes (G_BUILD_VECTOR, G_CONCAT_VECTOR, …) for vector support, it is important to have full types to avoid ambiguity.

Volkan

> 
>>> Otherwise, we’d need to mark it as Custom, check the full type and replace it with something else even if it’s legal.
>>> 
>>> Volkan
>>> 
>>>> 
>>>>> 
>>>>> 
>>>>> https://reviews.llvm.org/D39823 <https://reviews.llvm.org/D39823>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20171117/cfecfd68/attachment.html>