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

[Daniel Sanders via llvm-commits]

> On 17 Nov 2017, at 10:15, Quentin Colombet <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

>> 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/999b8e17/attachment.html>