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

[Quentin Colombet via llvm-commits]

> On Nov 13, 2017, at 8:51 AM, Kristof Beyls via Phabricator <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).


> 
> 
> https://reviews.llvm.org/D39823
> 
> 
>