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

[Quentin Colombet via llvm-commits]

> On Nov 14, 2017, at 4:43 PM, Volkan Keles <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?

> 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/ab159d49/attachment-0001.html>