[AArch64] Fix a build_vector pattern fail caused by defect in isBuildVectorAllZeros().

[kevin.qin]

Summary

Comments inside patch show the details. Please review, thanks.

[t.p.northover]

Hi Kevin,

Isn't this patch just hiding the backend issue? Enhancing isBuildVectorAllZeros like that seems like a good idea, but I can't see any guarantee that the problematic DAG can't still emerge by some other means (not that my quick tests have managed to produce it).

Cheers.

Tim.

[kevin.qin]

Hi Tim,

Thanks for your review. I enhanced isBuildVectorAllZeros() according to isBuildVectorAllones() which is just above it. I think it's meaningful to get it as robust as its brother function. For the word "problematic DAG", my understanding is you're referring to the constant larger than 16 bit width which is used to build a v4i16 vector. From my point of view, I'm not sure whether it's a "problematic DAG", because after legalization, DAG combiner may generate this kind of node in many paths. It's hard to avoid generating it unless we capture all paths that may create constant folding and guarantee those foldings only happen before legalization. I can do this for the path I observed, but I can't guarantee this kind of node won't be generated from other DAG combine path. So my suggestion is don't treat that node problematic, we should get our backend correctly handle that node as well.

Regards,
Kevin Qin

[t.p.northover]

Hi Kevin,

So my suggestion is don't treat that node problematic, we should get our backend correctly handle that node as well.

Agreed. That's exactly what I meant. It could come up via some other route so we need to be able to handle it.

Cheers.

Tim.

[kevin.qin]

Hi Tim,

It's committed in r211567. Cheers.

Kevin.

[t.p.northover]

Hi Kevin,

It's committed in r211567. Cheers.

That seems to be just the generic CodeGen improvement. What about the
actual AArch64 issue?

Cheers.

Tim.

[kevin.qin]

Sorry for following up this issue late. This patch is to normalize all constants building a vector. The value of constant nodes will be truncated to fit element width.

[t.p.northover]

Hi Kevin,

Thanks for the AArch64 patch. I think it looks reasonable.

Cheers.

Tim.

[Eugene.Zelenko]

Committed in rL212428.

lib/Target/AArch64/AArch64ISelLowering.cpp

   return Op;
 }
 
+// Normalize the operands of BUILD_VECTOR. The value of constant operands will
+// be truncated to fit element width.
+static SDValue NormalizeBuildVector(SDValue Op,
+                                    SelectionDAG &DAG) {
+  assert(Op.getOpcode() == ISD::BUILD_VECTOR && "Unknown opcode!");
+  SDLoc dl(Op);
+  EVT VT = Op.getValueType();
+  EVT EltTy= VT.getVectorElementType();
+
+  if (EltTy.isFloatingPoint() || EltTy.getSizeInBits() > 16)
+    return Op;
+
+  SmallVector<SDValue, 16> Ops;
+  for (unsigned I = 0, E = VT.getVectorNumElements(); I != E; ++I) {
+    SDValue Lane = Op.getOperand(I);
+    if (Lane.getOpcode() == ISD::Constant) {
+      APInt LowBits(EltTy.getSizeInBits(),
+                    cast<ConstantSDNode>(Lane)->getZExtValue());
+      Lane = DAG.getConstant(LowBits.getZExtValue(), MVT::i32);
+    }
+    Ops.push_back(Lane);
+  }
+  return DAG.getNode(ISD::BUILD_VECTOR, dl, VT, Ops);
+}
+
 SDValue AArch64TargetLowering::LowerBUILD_VECTOR(SDValue Op,
                                                  SelectionDAG &DAG) const {
-  BuildVectorSDNode *BVN = cast<BuildVectorSDNode>(Op.getNode());
   SDLoc dl(Op);
   EVT VT = Op.getValueType();
+  Op = NormalizeBuildVector(Op, DAG);
+  BuildVectorSDNode *BVN = cast<BuildVectorSDNode>(Op.getNode());
 
   APInt CnstBits(VT.getSizeInBits(), 0);
   APInt UndefBits(VT.getSizeInBits(), 0);

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

 
 define <8 x i16> @build_all_zero(<8 x i16> %a) #1 {
 ; CHECK-LABEL: build_all_zero:
-; CHECK: movn w[[GREG:[0-9]+]], #0x517f
+; CHECK: movz w[[GREG:[0-9]+]], #0xae80
 ; CHECK-NEXT: fmov  s[[FREG:[0-9]+]], w[[GREG]]
 ; CHECK-NEXT: mul.8h  v0, v0, v[[FREG]]
   %b = add <8 x i16> %a, <i16 -32768, i16 undef, i16 undef, i16 undef, i16 undef, i16 undef, i16 undef, i16 undef>