[GlobalISel] Better verification of G_UNMERGE_VALUES

[foad]

Summary

Verify three cases of G_UNMERGE_VALUES separately:

1. Splitting a vector into subvectors (the converse of G_CONCAT_VECTORS).
2. Splitting a vector into its elements (the converse of G_BUILD_VECTOR).
3. Splitting a scalar into smaller scalars (the converse of G_MERGE_VALUES).

[foad]

1. Splitting a vector into subvectors (the converse of G_CONCAT_VECTORS).

Is there any appetite for using a new (G_UNCONCAT_VECTORS???) opcode for this case?

Unfortunately there are a couple of tests that fail the verification I implemented for this case:

Failed Tests (2):

LLVM :: CodeGen/AArch64/GlobalISel/combine-unmerge.mir
LLVM :: CodeGen/AMDGPU/GlobalISel/artifact-combiner-unmerge-values.mir

by building MIR like this: %1:_(<2 x s16>), %2:_(<2 x s16>) = G_UNMERGE_VALUES %0:_(<2 x s32>)

Should this be allowed?

2. Splitting a vector into its elements (the converse of G_BUILD_VECTOR).

Is there any appetite for using a new G_SPLIT_VECTOR opcode for this case?

[aemerson]

In D111132#3042221, @foad wrote:

1. Splitting a vector into subvectors (the converse of G_CONCAT_VECTORS).

Is there any appetite for using a new (G_UNCONCAT_VECTORS???) opcode for this case?

Unfortunately there are a couple of tests that fail the verification I implemented for this case:

Failed Tests (2):

LLVM :: CodeGen/AArch64/GlobalISel/combine-unmerge.mir
LLVM :: CodeGen/AMDGPU/GlobalISel/artifact-combiner-unmerge-values.mir

by building MIR like this: %1:_(<2 x s16>), %2:_(<2 x s16>) = G_UNMERGE_VALUES %0:_(<2 x s32>)

Should this be allowed?

I don't like unmerge being able to do that. Seems better to handle that with a bitcast first.

2. Splitting a vector into its elements (the converse of G_BUILD_VECTOR).

Is there any appetite for using a new G_SPLIT_VECTOR opcode for this case?

As for splitting unmerge into separate opcodes, I don't have a particularly strong preference so I'm leaning towards keeping the status quo for now.

[arsenm]

In D111132#3042221, @foad wrote:

1. Splitting a vector into subvectors (the converse of G_CONCAT_VECTORS).

Is there any appetite for using a new (G_UNCONCAT_VECTORS???) opcode for this case?

Unfortunately there are a couple of tests that fail the verification I implemented for this case:

Failed Tests (2):

LLVM :: CodeGen/AArch64/GlobalISel/combine-unmerge.mir
LLVM :: CodeGen/AMDGPU/GlobalISel/artifact-combiner-unmerge-values.mir

by building MIR like this: %1:_(<2 x s16>), %2:_(<2 x s16>) = G_UNMERGE_VALUES %0:_(<2 x s32>)

Should this be allowed?

It probably shouldn't be

2. Splitting a vector into its elements (the converse of G_BUILD_VECTOR).

Is there any appetite for using a new G_SPLIT_VECTOR opcode for this case?

I don't see a reason to have a scalar and vector version of the same thing.

[foad]

In D111132#3071399, @arsenm wrote:

In D111132#3042221, @foad wrote:

2. Splitting a vector into its elements (the converse of G_BUILD_VECTOR).

Is there any appetite for using a new G_SPLIT_VECTOR opcode for this case?

I don't see a reason to have a scalar and vector version of the same thing.

You mean, splitting a vector into vectors vs splitting a vector into scalars? Then why do we have both G_BUILD_VECTOR and G_CONCAT_VECTORS? The asymmetry annoys me!

[foad]

Adjust MIR test cases to void G_UNMERGE_VALUES from vector to vector
with different element type.

[aemerson]

In D111132#3072726, @foad wrote:

In D111132#3071399, @arsenm wrote:

In D111132#3042221, @foad wrote:

2. Splitting a vector into its elements (the converse of G_BUILD_VECTOR).

Is there any appetite for using a new G_SPLIT_VECTOR opcode for this case?

I don't see a reason to have a scalar and vector version of the same thing.

You mean, splitting a vector into vectors vs splitting a vector into scalars? Then why do we have both G_BUILD_VECTOR and G_CONCAT_VECTORS? The asymmetry annoys me!

I'm not against splitting up unmerge into separate opcodes for symmetry. The original motivation of splitting G_MERGE was that the legalizer rules were very complex to deal with all the different cases that the instruction could handle.

[arsenm]

LGTM

llvm/lib/CodeGen/MachineVerifier.cpp

case TargetOpcode::G_MERGE_VALUES: {
     for (unsigned I = 2; I != NumOps; ++I) {
       if (MRI->getType(MI->getOperand(I).getReg()) != SrcTy)
         report("G_MERGE_VALUES source types do not match", MI);
     }
 
     break;
   }
   case TargetOpcode::G_UNMERGE_VALUES: {
+    unsigned NumDsts = MI->getNumOperands() - 1;
     LLT DstTy = MRI->getType(MI->getOperand(0).getReg());
-    LLT SrcTy = MRI->getType(MI->getOperand(MI->getNumOperands()-1).getReg());
-    // For now G_UNMERGE can split vectors.
-    for (unsigned i = 0; i < MI->getNumOperands()-1; ++i) {
-      if (MRI->getType(MI->getOperand(i).getReg()) != DstTy)
+    for (unsigned i = 1; i < NumDsts; ++i) {
+      if (MRI->getType(MI->getOperand(i).getReg()) != DstTy) {
         report("G_UNMERGE_VALUES destination types do not match", MI);
+        break;
+      }
     }
-    if (SrcTy.getSizeInBits() !=
-        (DstTy.getSizeInBits() * (MI->getNumOperands() - 1))) {
-      report("G_UNMERGE_VALUES source operand does not cover dest operands",
+
+    LLT SrcTy = MRI->getType(MI->getOperand(NumDsts).getReg());
+    if (DstTy.isVector()) {
+      // This case is the converse of G_CONCAT_VECTORS.
+      if (!SrcTy.isVector() || SrcTy.getScalarType() != DstTy.getScalarType() ||
+          SrcTy.getNumElements() != NumDsts * DstTy.getNumElements())
+        report("G_UNMERGE_VALUES source operand does not match vector "
+               "destination operands",
                MI);
+    } else if (SrcTy.isVector()) {
+      // This case is the converse of G_BUILD_VECTOR, but relaxed to allow
+      // mismatched types as long as the total size matches:
+      //   %0:_(s64), %1:_(s64) = G_UNMERGE_VALUES %2:_(<4 x s32>)
+      if (SrcTy.getSizeInBits() != NumDsts * DstTy.getSizeInBits())
+        report("G_UNMERGE_VALUES vector source operand does not match scalar "
+               "destination operands",
+               MI);
+    } else {
+      // This case is the converse of G_MERGE_VALUES.
+      if (SrcTy.getSizeInBits() != NumDsts * DstTy.getSizeInBits()) {
+        report("G_UNMERGE_VALUES scalar source operand does not match scalar "
+               "destination operands",
+               MI);
+      }
     }
     break;
   }
   case TargetOpcode::G_BUILD_VECTOR: {
     // Source types must be scalars, dest type a vector. Total size of scalars
     // must match the dest vector size.
     LLT DstTy = MRI->getType(MI->getOperand(0).getReg());
     LLT SrcEltTy = MRI->getType(MI->getOperand(1).getReg());

llvm/test/CodeGen/AArch64/GlobalISel/combine-unmerge.mir

 
 # Transform unmerge into trunc when only the first definition is live, even
 # if the input and output types are vectors.
 ---
 name:            test_combine_unmerge_dead_to_trunc_vec_in_n_out
 body:             |
   bb.1:
     ; CHECK-LABEL: name: test_combine_unmerge_dead_to_trunc_vec_in_n_out
-    ; CHECK: [[COPY:%[0-9]+]]:_(<2 x s32>) = COPY $x0
-    ; CHECK: [[BITCAST:%[0-9]+]]:_(s64) = G_BITCAST [[COPY]](<2 x s32>)
+    ; CHECK: [[COPY:%[0-9]+]]:_(<4 x s16>) = COPY $x0
+    ; CHECK: [[BITCAST:%[0-9]+]]:_(s64) = G_BITCAST [[COPY]](<4 x s16>)
     ; CHECK: [[TRUNC:%[0-9]+]]:_(s32) = G_TRUNC [[BITCAST]](s64)
     ; CHECK: [[BITCAST1:%[0-9]+]]:_(<2 x s16>) = G_BITCAST [[TRUNC]](s32)
     ; CHECK: $w0 = COPY [[BITCAST1]](<2 x s16>)
-    %0:_(<2 x s32>) = COPY $x0
-    %1:_(<2 x s16>),%2:_(<2 x s16>) = G_UNMERGE_VALUES %0(<2 x s32>)
+    %0:_(<4 x s16>) = COPY $x0
+    %1:_(<2 x s16>),%2:_(<2 x s16>) = G_UNMERGE_VALUES %0(<4 x s16>)
     $w0 = COPY %1(<2 x s16>)
 ...
 
 # Transform unmerge into trunc when only the first definition is live, even
 # if the input type is vector.
 ---
 name:            test_combine_unmerge_dead_to_trunc_vec_in
 body:             |
   bb.1:
     ; CHECK-LABEL: name: test_combine_unmerge_dead_to_trunc_vec_in
     ; CHECK: [[COPY:%[0-9]+]]:_(<2 x s32>) = COPY $x0
     ; CHECK: [[BITCAST:%[0-9]+]]:_(s64) = G_BITCAST [[COPY]](<2 x s32>)
     ; CHECK: [[TRUNC:%[0-9]+]]:_(s16) = G_TRUNC [[BITCAST]](s64)
     ; CHECK: $h0 = COPY [[TRUNC]](s16)
     %0:_(<2 x s32>) = COPY $x0
     %1:_(s16),%2:_(s16),%3:_(s16),%4:_(s16) = G_UNMERGE_VALUES %0(<2 x s32>)
     $h0 = COPY %1(s16)
 ...
 
-# Transform unmerge into trunc when only the first definition is live, even
-# if the output type are vector.
----
-name:            test_combine_unmerge_dead_to_trunc_vec_out
-body:             |
-  bb.1:
-    ; CHECK-LABEL: name: test_combine_unmerge_dead_to_trunc_vec_out
-    ; CHECK: [[COPY:%[0-9]+]]:_(s64) = COPY $x0
-    ; CHECK: [[TRUNC:%[0-9]+]]:_(s32) = G_TRUNC [[COPY]](s64)
-    ; CHECK: [[BITCAST:%[0-9]+]]:_(<2 x s16>) = G_BITCAST [[TRUNC]](s32)
-    ; CHECK: $w0 = COPY [[BITCAST]](<2 x s16>)
-    %0:_(s64) = COPY $x0
-    %1:_(<2 x s16>),%2:_(<2 x s16>) = G_UNMERGE_VALUES %0(s64)
-    $w0 = COPY %1(<2 x s16>)
-...
-
 # Transform unmerge(zext) into zext.
 # In that test, the source of the zext is same size as the first definition
 # of the unmerge. Therefore a we can just reuse the input of the zext for
 # this definition.
 ---
 name:            test_combine_unmerge_zext_to_zext_same_size
 body:             |
   bb.1:
bb.1:
     ; CHECK: $w0 = COPY [[UV]](s32)
     ; CHECK: $w1 = COPY [[UV1]](s32)
     %0:_(<2 x s16>) = COPY $w0
     %3:_(<2 x s32>) = G_ZEXT %0(<2 x s16>)
     %1:_(s32),%2:_(s32) = G_UNMERGE_VALUES %3(<2 x s32>)
     $w0 = COPY %1(s32)
     $w1 = COPY %2(s32)
 ...
-
-# Check that we don't apply the unmerge(zext) to zext transformation
-# when the destination type is a vector type.
-# We could actually handle this case but we would need to insert a cast.
----
-name:            test_dont_combine_unmerge_zext_to_zext_dst_vector
-body:             |
-  bb.1:
-    ; CHECK-LABEL: name: test_dont_combine_unmerge_zext_to_zext_dst_vector
-    ; CHECK: [[COPY:%[0-9]+]]:_(s32) = COPY $w0
-    ; CHECK: [[ZEXT:%[0-9]+]]:_(s64) = G_ZEXT [[COPY]](s32)
-    ; CHECK: [[UV:%[0-9]+]]:_(<2 x s16>), [[UV1:%[0-9]+]]:_(<2 x s16>) = G_UNMERGE_VALUES [[ZEXT]](s64)
-    ; CHECK: $w0 = COPY [[UV]](<2 x s16>)
-    ; CHECK: $w1 = COPY [[UV1]](<2 x s16>)
-    %0:_(s32) = COPY $w0
-    %3:_(s64) = G_ZEXT %0(s32)
-    %1:_(<2 x s16>),%2:_(<2 x s16>) = G_UNMERGE_VALUES %3(s64)
-    $w0 = COPY %1(<2 x s16>)
-    $w1 = COPY %2(<2 x s16>)
-...

llvm/test/CodeGen/AMDGPU/GlobalISel/artifact-combiner-unmerge-values.mir

bb.0:
     ; CHECK: [[ICMP:%[0-9]+]]:_(s1) = G_ICMP intpred(ne), [[UV]](s32), [[UV2]]
     ; CHECK: [[ICMP1:%[0-9]+]]:_(s1) = G_ICMP intpred(ne), [[UV1]](s32), [[UV3]]
     ; CHECK: [[ANYEXT:%[0-9]+]]:_(s16) = G_ANYEXT [[ICMP]](s1)
     ; CHECK: [[ANYEXT1:%[0-9]+]]:_(s16) = G_ANYEXT [[ICMP1]](s1)
     ; CHECK: [[ZEXT:%[0-9]+]]:_(s32) = G_ZEXT [[ANYEXT]](s16)
     ; CHECK: [[ZEXT1:%[0-9]+]]:_(s32) = G_ZEXT [[ANYEXT1]](s16)
     ; CHECK: [[ZEXT2:%[0-9]+]]:_(s32) = G_ZEXT [[ANYEXT]](s16)
     ; CHECK: [[ZEXT3:%[0-9]+]]:_(s32) = G_ZEXT [[ANYEXT1]](s16)
-    ; CHECK: [[BITCAST:%[0-9]+]]:_(<2 x s16>) = G_BITCAST [[ZEXT]](s32)
-    ; CHECK: [[BITCAST1:%[0-9]+]]:_(<2 x s16>) = G_BITCAST [[ZEXT1]](s32)
-    ; CHECK: [[BITCAST2:%[0-9]+]]:_(<2 x s16>) = G_BITCAST [[ZEXT2]](s32)
-    ; CHECK: [[BITCAST3:%[0-9]+]]:_(<2 x s16>) = G_BITCAST [[ZEXT3]](s32)
-    ; CHECK: S_ENDPGM 0, implicit [[BITCAST]](<2 x s16>), implicit [[BITCAST1]](<2 x s16>), implicit [[BITCAST2]](<2 x s16>), implicit [[BITCAST3]](<2 x s16>)
+    ; CHECK: [[BUILD_VECTOR:%[0-9]+]]:_(<2 x s32>) = G_BUILD_VECTOR [[ZEXT]](s32), [[ZEXT1]](s32)
+    ; CHECK: [[BUILD_VECTOR1:%[0-9]+]]:_(<2 x s32>) = G_BUILD_VECTOR [[ZEXT2]](s32), [[ZEXT3]](s32)
+    ; CHECK: S_ENDPGM 0, implicit [[BUILD_VECTOR]](<2 x s32>), implicit [[BUILD_VECTOR1]](<2 x s32>)
     %0:_(<2 x s32>) = COPY $vgpr0_vgpr1
     %1:_(<2 x s32>) = COPY $vgpr0_vgpr1
     %2:_(s32), %3:_(s32) = G_UNMERGE_VALUES %0(<2 x s32>)
     %4:_(s32), %5:_(s32) = G_UNMERGE_VALUES %1(<2 x s32>)
     %6:_(s1) = G_ICMP intpred(ne), %2(s32), %4
     %7:_(s1) = G_ICMP intpred(ne), %3(s32), %5
     %8:_(s16) = G_ANYEXT %6
     %9:_(s16) = G_ANYEXT %7
     %10:_(<4 x s16>) = G_BUILD_VECTOR %8, %9, %8, %9
     %11:_(<4 x s32>) = G_ZEXT %10
-    %12:_(<2 x s16>), %13:_(<2 x s16>), %14:_(<2 x s16>), %15:_(<2 x s16>) = G_UNMERGE_VALUES %11
-    S_ENDPGM 0, implicit %12, implicit %13, implicit %14, implicit %15
+    %12:_(<2 x s32>), %13:_(<2 x s32>) = G_UNMERGE_VALUES %11
+    S_ENDPGM 0, implicit %12, implicit %13
 
 ...
 
 ---
 name: test_unmerge_values_s1_trunc_v4s1_of_concat_vectors_v4s32_v2s32
 body:             |
   bb.0:
     ; CHECK-LABEL: name: test_unmerge_values_s1_trunc_v4s1_of_concat_vectors_v4s32_v2s32

llvm/test/MachineVerifier/test_g_unmerge_values.mir

+# RUN: not --crash llc -o - -march=arm64 -run-pass=none -verify-machineinstrs %s 2>&1 | FileCheck %s
+# REQUIRES: aarch64-registered-target
+
+---
+name: g_unmerge_values
+tracksRegLiveness: true
+body: |
+  bb.0:
+    ; CHECK: Bad machine code: G_UNMERGE_VALUES destination types do not match
+    %0:_(s64) = IMPLICIT_DEF
+    %1:_(s32), %2:_(s16) = G_UNMERGE_VALUES %0
+
+    ; CHECK: Bad machine code: G_UNMERGE_VALUES source operand does not match vector destination operands
+    %3:_(<4 x s32>) = IMPLICIT_DEF
+    %4:_(<3 x s32>), %5:_(<3 x s32>) = G_UNMERGE_VALUES %3
+
+    ; CHECK: Bad machine code: G_UNMERGE_VALUES source operand does not match vector destination operands
+    %6:_(<2 x s16>), %7:_(<2 x s16>) = G_UNMERGE_VALUES %3
+
+    ; CHECK: Bad machine code: G_UNMERGE_VALUES vector source operand does not match scalar destination operands
+    %8:_(<2 x s32>) = IMPLICIT_DEF
+    %9:_(s32), %10:_(s32), %11:_(s32) = G_UNMERGE_VALUES %8
+
+    ; CHECK: Bad machine code: G_UNMERGE_VALUES vector source operand does not match scalar destination operands
+    %12:_(s16), %13:_(s16) = G_UNMERGE_VALUES %8
+
+    ; CHECK: Bad machine code: G_UNMERGE_VALUES scalar source operand does not match scalar destination operands
+    %14:_(s64) = IMPLICIT_DEF
+    %15:_(s16), %16:_(s16) = G_UNMERGE_VALUES %14
+
+    ; CHECK: Bad machine code: G_UNMERGE_VALUES scalar source operand does not match scalar destination operands
+    %17:_(s32), %18:_(s32), %19:_(s32) = G_UNMERGE_VALUES %14
+...