[DAGCombine] Check the legality of the index of EXTRACT_SUBVECTOR

[guopeilin]

Summary

For ISD::EXTRACT_SUBVECTOR, its second operand must be a constant
multiple of the known-minimum vector length of the result type.

[david-arm]

llvm/test/CodeGen/AArch64/aarch64-avoid-illegal-extract-subvector.ll

9

Would it be possible to reduce this test a lot further? It looks like it shouldn't be too difficult to hand-write a new test that does something like:

%i1 = extractelement <4 x i32> %x, i32 1
%zi1 = zext i32 %i1 to i64
%i2 = extractelement <4 x i32> %x, i32 2
%zi2 = zext i32 %i2 to i64
%v1 = insertelement <2 x i64> undef, %zi1, i32 0
%v2 = insertelement <2 x i64> %v1, %zi2, i32 1

That looks like what the DAG combine is testing for and since the first extract is at index 1 it is not a multiple of the number of elements in the final build vector (<2 x i64>)

[RKSimon]

llvm/test/CodeGen/AArch64/aarch64-avoid-illegal-extract-subvector.ll

9

@guopeilin Any luck with simplifying this test?

[guopeilin]

Hi all, I update a new patch with a much simple test case. Please review, Thanks a lot!

[guopeilin]

Pinging reviewers....

[RKSimon]

llvm/test/CodeGen/AArch64/aarch64-avoid-illegal-extract-subvector.ll

9

Can this be cleaned up anymore - the dso_local / local_unnamed_addr attributes seem superfluous for instance.

[dmgreen]

Do all the checks pass with this? The precommit checks suggest that an AMD test is failing.

llvm/test/CodeGen/AArch64/aarch64-avoid-illegal-extract-subvector.ll

9

Can you also generate the checks with the script. The file says it's generated with update_llc_test_checks, but these checks look sparse. Can you also add the testcase David talked about:

define <2 x i64> @test(<4 x i32> %x) #0 {
  %i1 = extractelement <4 x i32> %x, i32 1
  %zi1 = zext i32 %i1 to i64
  %i2 = extractelement <4 x i32> %x, i32 2
  %zi2 = zext i32 %i2 to i64
  %v1 = insertelement <2 x i64> undef, i64 %zi1, i32 0
  %v2 = insertelement <2 x i64> %v1, i64 %zi2, i32 1
  ret <2 x i64> %v2
}

It won't fail, but does show the same unaligned EXTRACT_SUBVECTOR. I guess that at some point worked for unaligned extract offsets.

[guopeilin]

Hi all!
Updata a new test case.
Function test2 will fail due to assertion of invalid offset of EXTRACT_SUBVECTOR.
Also modify the AMD test case

[dmgreen]

Thanks. Looks good to me, as per the definition of EXTRACT_SUBVECTOR.

llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp

if ((Opc == ISD::ZERO_EXTEND || Opc == ISD::ANY_EXTEND) &&
         Op0.getOperand(0).getOperand(0) == Op.getOperand(0).getOperand(0))
       if (auto *C = dyn_cast<ConstantSDNode>(Op.getOperand(0).getOperand(1)))
         return C->getZExtValue();
     return -1;
   };
 
   // Make sure the first element matches
   // (zext (extract_vector_elt X, C))
+  // Offset must be a constant multiple of the
+  // known-minimum vector length of the result type.
   int64_t Offset = checkElem(Op0);
-  if (Offset < 0)
+  if (Offset < 0 || (Offset % VT.getVectorNumElements()) != 0)
     return SDValue();
 
   unsigned NumElems = N->getNumOperands();
   SDValue In = Op0.getOperand(0).getOperand(0);
   EVT InSVT = In.getValueType().getScalarType();
   EVT InVT = EVT::getVectorVT(*DAG.getContext(), InSVT, NumElems);
 
   // Don't create an illegal input type after type legalization.

llvm/test/CodeGen/AArch64/aarch64-avoid-illegal-extract-subvector.ll

+; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
+; RUN: llc -mtriple=aarch64-none-linux-gnu < %s -o -| FileCheck %s
+define <2 x i64> @test1(<4 x i32> %x) #0 {
+; CHECK-LABEL: test1:
+; CHECK:       // %bb.0:
+; CHECK-NEXT:    mov w8, v0.s[1]
+; CHECK-NEXT:    mov w9, v0.s[2]
+; CHECK-NEXT:    fmov d0, x8
+; CHECK-NEXT:    mov v0.d[1], x9
+; CHECK-NEXT:    ret
+  %i1 = extractelement <4 x i32> %x, i32 1
+  %zi1 = zext i32 %i1 to i64
+  %i2 = extractelement <4 x i32> %x, i32 2
+  %zi2 = zext i32 %i2 to i64
+  %v1 = insertelement <2 x i64> undef, i64 %zi1, i32 0
+  %v2 = insertelement <2 x i64> %v1, i64 %zi2, i32 1
+  ret <2 x i64> %v2
+}
+
+define <4 x i64> @test2(<4 x i32> %0) {
+; CHECK-LABEL: test2:
+; CHECK:       // %bb.0: // %entry
+; CHECK-NEXT:    adrp x8, .LCPI1_0
+; CHECK-NEXT:    ldr q1, [x8, :lo12:.LCPI1_0]
+; CHECK-NEXT:    add v0.4s, v0.4s, v1.4s
+; CHECK-NEXT:    mov w8, v0.s[1]
+; CHECK-NEXT:    mov w9, v0.s[2]
+; CHECK-NEXT:    fmov d1, x8
+; CHECK-NEXT:    mov v1.d[1], x9
+; CHECK-NEXT:    ret
+entry:
+  %1 = add <4 x i32> %0, <i32 -4, i32 -8, i32 -12, i32 -16>
+  %2 = extractelement <4 x i32> %1, i32 1
+  %zext1 = zext i32 %2 to i64
+  %3 = extractelement <4 x i32> %1, i32 2
+  %zext2 = zext i32 %3 to i64
+  %4 = insertelement <4 x i64> undef, i64 %zext1, i32 2
+  %5 = insertelement <4 x i64> %4, i64 %zext2, i32 3
+  ret <4 x i64> %5
+}

llvm/test/CodeGen/AMDGPU/llvm.amdgcn.struct.buffer.load.format.v3f16.ll

 ; GFX8-NEXT:    buffer_load_format_d16_xyz v[5:6], v4, s[4:7], 0 idxen
 ; GFX8-NEXT:    ; implicit-def: $vgpr0_vgpr1_vgpr2_vgpr3
 ; GFX8-NEXT:    ; implicit-def: $vgpr4
 ; GFX8-NEXT:    s_xor_b64 exec, exec, s[0:1]
 ; GFX8-NEXT:    s_cbranch_execnz BB0_1
 ; GFX8-NEXT:  ; %bb.2:
 ; GFX8-NEXT:    s_mov_b64 exec, s[2:3]
 ; GFX8-NEXT:    s_waitcnt vmcnt(0)
-; GFX8-NEXT:    v_alignbit_b32 v0, v6, v5, 16
-; GFX8-NEXT:    v_lshrrev_b32_e32 v1, 16, v0
-; GFX8-NEXT:    v_and_b32_e32 v0, 0xffff, v0
+; GFX8-NEXT:    v_lshrrev_b32_e32 v0, 16, v5
+; GFX8-NEXT:    v_and_b32_e32 v1, 0xffff, v6
 ; GFX8-NEXT:    v_mov_b32_e32 v2, 0
 ; GFX8-NEXT:    s_mov_b32 m0, -1
 ; GFX8-NEXT:    ds_write2_b32 v2, v0, v1 offset0:7 offset1:8
 bb:
   %i = call i32 @llvm.amdgcn.mbcnt.hi(i32 -1, i32 undef)
   %i2 = call nsz arcp <3 x half> @llvm.amdgcn.struct.buffer.load.format.v3f16(<4 x i32> %arg, i32 %arg1, i32 0, i32 0, i32 0)
   %i3 = bitcast <3 x half> %i2 to <3 x i16>
   %i4 = extractelement <3 x i16> %i3, i32 1