[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

8

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

8

@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

8

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

8

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

SDValue DAGCombiner::convertBuildVecZextToZext(SDNode *N) {
 
   // Make sure the first element matches
   // (zext (extract_vector_elt X, C))
   int64_t Offset = checkElem(Op0);
   if (Offset < 0)
     return SDValue();
 
   unsigned NumElems = N->getNumOperands();
+  // Offset must be a constant multiple of the
+  // known-minimum vector length of the result type.
+  if (Offset % NumElems != 0)
+    return SDValue();
+
   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.
   if (LegalTypes && !TLI.isTypeLegal(InVT))
     return SDValue();
 

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
+
+@e = external dso_local local_unnamed_addr global i32, align 4
+@a = external dso_local local_unnamed_addr global i16, align 4
+@f = external dso_local local_unnamed_addr global i32, align 4
+
+define dso_local void @h() local_unnamed_addr {
+; CHECK-LABEL: h:
+; CHECK:       // %bb.0: // %entry
+; CHECK:         addv h0, v0.8h
+;
+entry:
+  %f.promoted153 = load i32, i32* @f, align 4
+  %0 = load i32, i32* @e, align 4
+  %tobool16 = icmp ne i32 %0, 0
+  %1 = insertelement <8 x i1> undef, i1 %tobool16, i32 0
+  %2 = shufflevector <8 x i1> %1, <8 x i1> undef, <8 x i32> zeroinitializer
+  %conv6 = zext i32 %f.promoted153 to i64
+  %div7 = sdiv i64 undef, %conv6
+  %3 = insertelement <8 x i32> undef, i32 %f.promoted153, i32 0
+  %4 = shufflevector <8 x i32> %3, <8 x i32> undef, <8 x i32> zeroinitializer
+  %5 = add <8 x i32> %4, <i32 -4, i32 -8, i32 -12, i32 -16, i32 -20, i32 -24, i32 -28, i32 -32>
+  %6 = extractelement <8 x i32> %5, i32 1
+  %conv18.2 = zext i32 %6 to i64
+  %7 = extractelement <8 x i32> %5, i32 2
+  %conv18.3 = zext i32 %7 to i64
+  %8 = insertelement <8 x i64> undef, i64 %div7, i32 0
+  %9 = shufflevector <8 x i64> %8, <8 x i64> undef, <8 x i32> zeroinitializer
+  %10 = insertelement <8 x i64> undef, i64 %conv18.2, i32 2
+  %11 = insertelement <8 x i64> %10, i64 %conv18.3, i32 3
+  %12 = insertelement <8 x i64> %11, i64 undef, i32 4
+  %13 = insertelement <8 x i64> %12, i64 undef, i32 5
+  %14 = insertelement <8 x i64> %13, i64 undef, i32 6
+  %15 = insertelement <8 x i64> %14, i64 undef, i32 7
+  %16 = icmp ne <8 x i64> %9, %15
+  %17 = and <8 x i1> %16, %2
+  %18 = zext <8 x i1> %17 to <8 x i32>
+  %19 = icmp ne <8 x i32> %5, %18
+  %20 = sext <8 x i1> %19 to <8 x i16>
+  %conv22.neg.8 = sext i1 undef to i16
+  %conv22.neg.9 = sext i1 undef to i16
+  %21 = call i16 @llvm.vector.reduce.add.v8i16(<8 x i16> %20)
+  %22 = add i16 %21, %conv22.neg.8
+  %23 = add i16 %22, %conv22.neg.9
+  %op.extra = add i16 %23, 5
+  %conv22.neg.10 = sext i1 undef to i16
+  %sub24.10 = add i16 %op.extra, %conv22.neg.10
+  store i16 %sub24.10, i16* @a, align 4
+  ret void
+}
+
+declare i16 @llvm.vector.reduce.add.v8i16(<8 x i16>)