[AArch64] Fix assertion failure on widened f16 BUILD_VECTOR

[bryanpkc]

Summary

Ensure that NormalizedBuildVector returns a BUILD_VECTOR with operands of the
same type. This fixes an assertion failure in VerifySDNode.

[SjoerdMeijer]

lib/Target/AArch64/AArch64ISelLowering.cpp

6896 ↗

(On Diff #158851)

Doesn't look like you're testing this; add a test for this?

test/CodeGen/AArch64/arm64-build-vector.ll

43 ↗

(On Diff #158851)

Nit: perhaps move some of these explanations to the implementation in NormalizeBuildVector? But definitely a brief explanation of the test would be good.

[bryanpkc]

lib/Target/AArch64/AArch64ISelLowering.cpp

6896 ↗

(On Diff #158851)

Actually the test is exercising this path; widening adds i16 UNDEF nodes to a vector, but when normalizing the vector, they were left alone and not promoted.

The next block is in fact a preemptive fix, for which I couldn't construct a test case. However, zero-extending a 16-bit or smaller operand in an integer BUILD_VECTOR to 32 bits seems pretty safe to me, and is consistent with what we already do for constant operands in this function.

test/CodeGen/AArch64/arm64-build-vector.ll

43 ↗

(On Diff #158851)

OK, will do.

[bryanpkc]

Removed impossible code path from NormalizeBuildVector.

[SjoerdMeijer]

Thanks, looks reasonable to me.

llvm/trunk/lib/Target/AArch64/AArch64ISelLowering.cpp

static SDValue NormalizeBuildVector(SDValue Op,
   EVT VT = Op.getValueType();
   EVT EltTy= VT.getVectorElementType();
 
   if (EltTy.isFloatingPoint() || EltTy.getSizeInBits() > 16)
     return Op;
 
   SmallVector<SDValue, 16> Ops;
   for (SDValue Lane : Op->ops()) {
+    // For integer vectors, type legalization would have promoted the
+    // operands already. Otherwise, if Op is a floating-point splat
+    // (with operands cast to integers), then the only possibilities
+    // are constants and UNDEFs.
     if (auto *CstLane = dyn_cast<ConstantSDNode>(Lane)) {
       APInt LowBits(EltTy.getSizeInBits(),
                     CstLane->getZExtValue());
       Lane = DAG.getConstant(LowBits.getZExtValue(), dl, MVT::i32);
+    } else if (Lane.getNode()->isUndef()) {
+      Lane = DAG.getUNDEF(MVT::i32);
+    } else {
+      assert(Lane.getValueType() == MVT::i32 &&
+             "Unexpected BUILD_VECTOR operand type");
     }
     Ops.push_back(Lane);
   }
   return DAG.getBuildVector(VT, dl, Ops);
 }
 
 static SDValue ConstantBuildVector(SDValue Op, SelectionDAG &DAG) {
   EVT VT = Op.getValueType();

llvm/trunk/test/CodeGen/AArch64/arm64-build-vector.ll

 define <8 x i16> @concat_2_build_vector(<4 x i16> %in0) {
 ; CHECK-LABEL: concat_2_build_vector:
 ; CHECK: movi
   %vshl_n = shl <4 x i16> %in0, <i16 8, i16 8, i16 8, i16 8>
   %vshl_n2 = shl <4 x i16> %vshl_n, <i16 9, i16 9, i16 9, i16 9>
   %shuffle.i = shufflevector <4 x i16> %vshl_n2, <4 x i16> zeroinitializer, <8 x i32> <i32 0, i32 1, i32 2, i32 3, i32 4, i32 5, i32 6, i32 7>
   ret <8 x i16> %shuffle.i
 }
+
+; The lowering of a widened f16 BUILD_VECTOR tries to optimize it by building
+; an equivalent integer vector and BITCAST-ing that. This case checks that
+; normalizing the vector generates a valid result. The choice of the
+; constant prevents earlier passes from replacing the BUILD_VECTOR.
+define void @widen_f16_build_vector(half* %addr) {
+; CHECK-LABEL: widen_f16_build_vector:
+; CHECK: mov    w[[GREG:[0-9]+]], #13294
+; CHECK: dup.4h v0, w[[GREG]]
+; CHECK: str    s0, [x0]
+  %1 = bitcast half* %addr to <2 x half>*
+  store <2 x half> <half 0xH33EE, half 0xH33EE>, <2 x half>* %1, align 2
+  ret void
+}