[ARM] More aggressive matching for vpadd and vpaddl.

[efriedma]

Summary

This seems to mostly work, but I'm not sure I'm doing it at the right layer. Maybe we should be trying to match this before shuffle lowering?

[rengolin]

Generally, looks good. I have some comments on the format of the code, which could be a bit cleaner on the depths of the ifs and commoning up the checks.

cheers,
--renato

lib/Target/ARM/ARMISelLowering.cpp

9219

Why v2i32 only? I can see from your tests that this also matches i8, so I'm suspecting this is already extended by the selection dag before getting here?

Also, VPADD works with floats, too.

9221

Better to negate this if and move return SDValue() here.

9252

This if is almost identical to the one above. I'd common them up in a local static function.

9256

VPADDL works on D and Q regs, not Q only.

9257

Same comment here, better to negate this if and return SDValue().

[efriedma]

lib/Target/ARM/ARMISelLowering.cpp

9219

This conditional is kind of awkward; I really want to just look for VUZP, but there is no v2i32 VUZP.

(The float vpadd isn't quite the same thing, but I guess I can look into it.)

9256

Yes... but in that case the type of N00 would be a 32-bit vector, and the whole thing kind of explodes due to the way legalization works. I'll include a testcase for reference.

[efriedma]

Refactor code; add testcases.

[efriedma]

Don't review the new version yet; I have more changes.

[efriedma]

More testcases, some refactoring, bugfix for a stupid mistake.

[rengolin]

lib/Target/ARM/ARMISelLowering.cpp

9621

This looks weird. As much as I have long methods and nested ifs, the interaction between Early and Late increases the coupling between them and make calling code more complex. (for example, you have an early exit on Early for later match on Later).

Adding an if block in AddCombineToVPADDL with an return would be much cleaner, if the condition is clear and concise, which should be the case, with your new helper.

[efriedma]

lib/Target/ARM/ARMISelLowering.cpp

9621

They aren't really coupled like that; when I say "we'll match it to vpadd later", I mean, way later, like after the BUILD_VECTOR is lowered by legalization. Maybe I could rename "Early" and "Late" to be a bit more clear...

Also, my eventual intent is to kill off AddCombineToVPADDLEarly; I'm just leaving it around to avoid regressing the fromExtendingExtractVectorElt_i8 and fromExtendingExtractVectorElt_i16 cases, which get lowered in stupid ways otherwise.

[rengolin]

lib/Target/ARM/ARMISelLowering.cpp

9219

Oh, I see! ok.

9621

Right, I see what you mean. If you rename to what they match (instead of early/late), they'll become two different calls anyway, just like the rest of them in this function.

It'd also good to add a FIXME on the current Early one, so anyone looking will know.

[efriedma]

Updated comments and function names.

[rengolin]

This looks much better now, thanks! LGTM.

lib/Target/ARM/ARMISelLowering.cpp

if (N0.getNode()->hasOneUse())
     if (SDValue Result = combineSelectAndUse(N, N0, N1, DCI, AllOnes))
       return Result;
   if (N1.getNode()->hasOneUse())
     if (SDValue Result = combineSelectAndUse(N, N1, N0, DCI, AllOnes))
       return Result;
   return SDValue();
 }
 
+// AddCombineToVPADD - For pair-wise add on neon, use the vpadd instruction.
+static SDValue AddCombineToVPADD(SDNode *N, SDValue N0, SDValue N1,
+                                 TargetLowering::DAGCombinerInfo &DCI,
+                                 const ARMSubtarget *Subtarget) {
+  if ((N0.getOpcode() == ARMISD::VUZP ||
+      (N0.getOpcode() == ARMISD::VTRN && N0.getValueType() == MVT::v2i32)) &&
+      N0.getNode() == N1.getNode() &&
+      N0 != N1 && N->getValueType(0).is64BitVector()) {
+    // An add where the inputs both come from the same vuzp forms a vpadd.
+    SelectionDAG &DAG = DCI.DAG;
+    const TargetLowering &TLI = DAG.getTargetLoweringInfo();
+    SDLoc dl(N);
+    SDNode *Unzip = N0.getNode();
+    EVT VT = N->getValueType(0);
+
+    SmallVector<SDValue, 8> Ops;
+    Ops.push_back(DAG.getConstant(Intrinsic::arm_neon_vpadd, dl,
+                                  TLI.getPointerTy(DAG.getDataLayout())));
+    Ops.push_back(Unzip->getOperand(0));
+    Ops.push_back(Unzip->getOperand(1));
+
+    return DAG.getNode(ISD::INTRINSIC_WO_CHAIN, dl, VT, Ops);
+  }
+
+  return SDValue();
+}
+
 // AddCombineToVPADDL- For pair-wise add on neon, use the vpaddl instruction
 // (only after legalization).
 static SDValue AddCombineToVPADDL(SDNode *N, SDValue N0, SDValue N1,
                                  TargetLowering::DAGCombinerInfo &DCI,
                                  const ARMSubtarget *Subtarget) {
+  if ((N0.getOpcode() == ISD::SIGN_EXTEND &&
+       N1.getOpcode() == ISD::SIGN_EXTEND) ||
+      (N0.getOpcode() == ISD::ZERO_EXTEND &&
+       N1.getOpcode() == ISD::ZERO_EXTEND)) {
+    SDValue N00 = N0.getOperand(0);
+    SDValue N10 = N1.getOperand(0);
+    if ((N00.getOpcode() == ARMISD::VUZP ||
+        (N00.getOpcode() == ARMISD::VTRN &&
+         N00.getValueType() == MVT::v2i32)) &&
+        N00.getNode() == N10.getNode() &&
+        N00 != N10 && N00.getValueType().is64BitVector() &&
+        N0.getValueType().is128BitVector()) {
+      // An extended add where the inputs both come from the same vuzp forms
+      // a vpaddl.
+      SelectionDAG &DAG = DCI.DAG;
+      const TargetLowering &TLI = DAG.getTargetLoweringInfo();
+      SDLoc dl(N);
+      EVT VT = N->getValueType(0);
+
+      SmallVector<SDValue, 8> Ops;
+      // Form vpaddl.sN or vpaddl.uN depending on the kind of extension.
+      unsigned Opcode;
+      if (N0.getOpcode() == ISD::SIGN_EXTEND)
+        Opcode = Intrinsic::arm_neon_vpaddls;
+      else
+        Opcode = Intrinsic::arm_neon_vpaddlu;
+      Ops.push_back(DAG.getConstant(Opcode, dl,
+                                    TLI.getPointerTy(DAG.getDataLayout())));
+      EVT ElemTy = N00.getValueType().getVectorElementType();
+      unsigned NumElts = VT.getVectorNumElements();
+      EVT ConcatVT = EVT::getVectorVT(*DAG.getContext(), ElemTy, NumElts * 2);
+      SDValue Concat = DAG.getNode(ISD::CONCAT_VECTORS, SDLoc(N), ConcatVT,
+                                   N00, N10);
+      Ops.push_back(Concat);
+
+      return DAG.getNode(ISD::INTRINSIC_WO_CHAIN, dl, VT, Ops);
+
+    }
+  }
+
+  // FIXME: We shouldn't produce a DAG which matches the following pattern.
+  // There are more efficient ways to lower this sort of shuffle in the
+  // general case.
 
   // Only perform optimization if after legalize, and if NEON is available. We
   // also expected both operands to be BUILD_VECTORs.
   if (DCI.isBeforeLegalize() || !Subtarget->hasNEON()
       || N0.getOpcode() != ISD::BUILD_VECTOR
       || N1.getOpcode() != ISD::BUILD_VECTOR)
     return SDValue();
 

 
 /// PerformADDCombineWithOperands - Try DAG combinations for an ADD with
 /// operands N0 and N1.  This is a helper for PerformADDCombine that is
 /// called with the default operands, and if that fails, with commuted
 /// operands.
 static SDValue PerformADDCombineWithOperands(SDNode *N, SDValue N0, SDValue N1,
                                           TargetLowering::DAGCombinerInfo &DCI,
                                           const ARMSubtarget *Subtarget){
+  // Attempt to create vpadd for this add.
+  if (SDValue Result = AddCombineToVPADD(N, N0, N1, DCI, Subtarget))
+    return Result;
 
   // Attempt to create vpaddl for this add.
   if (SDValue Result = AddCombineToVPADDL(N, N0, N1, DCI, Subtarget))
     return Result;
 
   // fold (add (select cc, 0, c), x) -> (select cc, x, (add, x, c))
   if (N0.getNode()->hasOneUse())
     if (SDValue Result = combineSelectAndUse(N, N0, N1, DCI))
       return Result;
   return SDValue();
 }

test/CodeGen/ARM/vpadd.ll

 ;CHECK-LABEL: vpaddlQu32:
 ;CHECK: vpaddl.u32
   %tmp1 = load <4 x i32>, <4 x i32>* %A
   %tmp2 = call <2 x i64> @llvm.arm.neon.vpaddlu.v2i64.v4i32(<4 x i32> %tmp1)
   ret <2 x i64> %tmp2
 }
 
 ; Combine vuzp+vadd->vpadd.
-; FIXME: Implement this optimization
-define void @addCombineToVPADD(<16 x i8> *%cbcr, <8 x i8> *%X) nounwind ssp {
-; CHECK-LABEL: addCombineToVPADD:
-; CHECK: vuzp.8
-; CHECK: vadd.i8
+define void @addCombineToVPADD_i8(<16 x i8> *%cbcr, <8 x i8> *%X) nounwind ssp {
+; CHECK-LABEL: addCombineToVPADD_i8:
+; CHECK: vpadd.i8
   %tmp = load <16 x i8>, <16 x i8>* %cbcr
   %tmp1 = shufflevector <16 x i8> %tmp, <16 x i8> undef, <8 x i32> <i32 0, i32 2, i32 4, i32 6, i32 8, i32 10, i32 12, i32 14>
   %tmp3 = shufflevector <16 x i8> %tmp, <16 x i8> undef, <8 x i32> <i32 1, i32 3, i32 5, i32 7, i32 9, i32 11, i32 13, i32 15>
   %add = add <8 x i8> %tmp3, %tmp1
   store <8 x i8> %add, <8 x i8>* %X, align 8
   ret void
 }
 
+; Combine vuzp+vadd->vpadd.
+define void @addCombineToVPADD_i16(<8 x i16> *%cbcr, <4 x i16> *%X) nounwind ssp {
+; CHECK-LABEL: addCombineToVPADD_i16:
+; CHECK: vpadd.i16
+  %tmp = load <8 x i16>, <8 x i16>* %cbcr
+  %tmp1 = shufflevector <8 x i16> %tmp, <8 x i16> undef, <4 x i32> <i32 0, i32 2, i32 4, i32 6>
+  %tmp3 = shufflevector <8 x i16> %tmp, <8 x i16> undef, <4 x i32> <i32 1, i32 3, i32 5, i32 7>
+  %add = add <4 x i16> %tmp3, %tmp1
+  store <4 x i16> %add, <4 x i16>* %X, align 8
+  ret void
+}
+
+; Combine vtrn+vadd->vpadd.
+define void @addCombineToVPADD_i32(<4 x i32> *%cbcr, <2 x i32> *%X) nounwind ssp {
+; CHECK-LABEL: addCombineToVPADD_i32:
+; CHECK: vpadd.i32
+  %tmp = load <4 x i32>, <4 x i32>* %cbcr
+  %tmp1 = shufflevector <4 x i32> %tmp, <4 x i32> undef, <2 x i32> <i32 0, i32 2>
+  %tmp3 = shufflevector <4 x i32> %tmp, <4 x i32> undef, <2 x i32> <i32 1, i32 3>
+  %add = add <2 x i32> %tmp3, %tmp1
+  store <2 x i32> %add, <2 x i32>* %X, align 8
+  ret void
+}
+
 ; Combine vuzp+vaddl->vpaddl
-; FIXME: Implement this optimization.
-define void @addCombineToVPADDL_sext(<16 x i8> *%cbcr, <8 x i16> *%X) nounwind ssp {
-; CHECK-LABEL: addCombineToVPADDL_sext:
-; CHECK: vuzp.8
-; CHECK: vaddl.s8
+define void @addCombineToVPADDLq_s8(<16 x i8> *%cbcr, <8 x i16> *%X) nounwind ssp {
+; CHECK-LABEL: addCombineToVPADDLq_s8:
+; CHECK: vpaddl.s8
   %tmp = load <16 x i8>, <16 x i8>* %cbcr
   %tmp1 = shufflevector <16 x i8> %tmp, <16 x i8> undef, <8 x i32> <i32 0, i32 2, i32 4, i32 6, i32 8, i32 10, i32 12, i32 14>
   %tmp3 = shufflevector <16 x i8> %tmp, <16 x i8> undef, <8 x i32> <i32 1, i32 3, i32 5, i32 7, i32 9, i32 11, i32 13, i32 15>
   %tmp4 = sext <8 x i8> %tmp3 to <8 x i16>
   %tmp5 = sext <8 x i8> %tmp1 to <8 x i16>
   %add = add <8 x i16> %tmp4, %tmp5
   store <8 x i16> %add, <8 x i16>* %X, align 8
   ret void
 }
 
-; Legalization produces a EXTRACT_VECTOR_ELT DAG node which performs an extend from
-; i16 to i32. In this case the input for the formed VPADDL needs to be a vector of i16s.
-define <2 x i16> @fromExtendingExtractVectorElt(<4 x i16> %in) {
-;CHECK-LABEL: fromExtendingExtractVectorElt:
+; Combine vuzp+vaddl->vpaddl
+define void @addCombineToVPADDLq_u8(<16 x i8> *%cbcr, <8 x i16> *%X) nounwind ssp {
+; CHECK-LABEL: addCombineToVPADDLq_u8:
+; CHECK: vpaddl.u8
+  %tmp = load <16 x i8>, <16 x i8>* %cbcr
+  %tmp1 = shufflevector <16 x i8> %tmp, <16 x i8> undef, <8 x i32> <i32 0, i32 2, i32 4, i32 6, i32 8, i32 10, i32 12, i32 14>
+  %tmp3 = shufflevector <16 x i8> %tmp, <16 x i8> undef, <8 x i32> <i32 1, i32 3, i32 5, i32 7, i32 9, i32 11, i32 13, i32 15>
+  %tmp4 = zext <8 x i8> %tmp3 to <8 x i16>
+  %tmp5 = zext <8 x i8> %tmp1 to <8 x i16>
+  %add = add <8 x i16> %tmp4, %tmp5
+  store <8 x i16> %add, <8 x i16>* %X, align 8
+  ret void
+}
+
+; Combine vuzp+vaddl->vpaddl
+define void @addCombineToVPADDLq_s16(<8 x i16> *%cbcr, <4 x i32> *%X) nounwind ssp {
+; CHECK-LABEL: addCombineToVPADDLq_s16:
+; CHECK: vpaddl.s16
+  %tmp = load <8 x i16>, <8 x i16>* %cbcr
+  %tmp1 = shufflevector <8 x i16> %tmp, <8 x i16> undef, <4 x i32> <i32 0, i32 2, i32 4, i32 6>
+  %tmp3 = shufflevector <8 x i16> %tmp, <8 x i16> undef, <4 x i32> <i32 1, i32 3, i32 5, i32 7>
+  %tmp4 = sext <4 x i16> %tmp3 to <4 x i32>
+  %tmp5 = sext <4 x i16> %tmp1 to <4 x i32>
+  %add = add <4 x i32> %tmp4, %tmp5
+  store <4 x i32> %add, <4 x i32>* %X, align 8
+  ret void
+}
+
+; Combine vuzp+vaddl->vpaddl
+define void @addCombineToVPADDLq_u16(<8 x i16> *%cbcr, <4 x i32> *%X) nounwind ssp {
+; CHECK-LABEL: addCombineToVPADDLq_u16:
+; CHECK: vpaddl.u16
+  %tmp = load <8 x i16>, <8 x i16>* %cbcr
+  %tmp1 = shufflevector <8 x i16> %tmp, <8 x i16> undef, <4 x i32> <i32 0, i32 2, i32 4, i32 6>
+  %tmp3 = shufflevector <8 x i16> %tmp, <8 x i16> undef, <4 x i32> <i32 1, i32 3, i32 5, i32 7>
+  %tmp4 = zext <4 x i16> %tmp3 to <4 x i32>
+  %tmp5 = zext <4 x i16> %tmp1 to <4 x i32>
+  %add = add <4 x i32> %tmp4, %tmp5
+  store <4 x i32> %add, <4 x i32>* %X, align 8
+  ret void
+}
+
+; Combine vtrn+vaddl->vpaddl
+define void @addCombineToVPADDLq_s32(<4 x i32> *%cbcr, <2 x i64> *%X) nounwind ssp {
+; CHECK-LABEL: addCombineToVPADDLq_s32:
+; CHECK: vpaddl.s32
+  %tmp = load <4 x i32>, <4 x i32>* %cbcr
+  %tmp1 = shufflevector <4 x i32> %tmp, <4 x i32> undef, <2 x i32> <i32 0, i32 2>
+  %tmp3 = shufflevector <4 x i32> %tmp, <4 x i32> undef, <2 x i32> <i32 1, i32 3>
+  %tmp4 = sext <2 x i32> %tmp3 to <2 x i64>
+  %tmp5 = sext <2 x i32> %tmp1 to <2 x i64>
+  %add = add <2 x i64> %tmp4, %tmp5
+  store <2 x i64> %add, <2 x i64>* %X, align 8
+  ret void
+}
+
+; Combine vtrn+vaddl->vpaddl
+define void @addCombineToVPADDLq_u32(<4 x i32> *%cbcr, <2 x i64> *%X) nounwind ssp {
+; CHECK-LABEL: addCombineToVPADDLq_u32:
+; CHECK: vpaddl.u32
+  %tmp = load <4 x i32>, <4 x i32>* %cbcr
+  %tmp1 = shufflevector <4 x i32> %tmp, <4 x i32> undef, <2 x i32> <i32 0, i32 2>
+  %tmp3 = shufflevector <4 x i32> %tmp, <4 x i32> undef, <2 x i32> <i32 1, i32 3>
+  %tmp4 = zext <2 x i32> %tmp3 to <2 x i64>
+  %tmp5 = zext <2 x i32> %tmp1 to <2 x i64>
+  %add = add <2 x i64> %tmp4, %tmp5
+  store <2 x i64> %add, <2 x i64>* %X, align 8
+  ret void
+}
+
+; Legalization promotes the <4 x i8> to <4 x i16>.
+define <4 x i8> @fromExtendingExtractVectorElt_i8(<8 x i8> %in) {
+;CHECK-LABEL: fromExtendingExtractVectorElt_i8:
+;CHECK: vpaddl.s8
+  %tmp1 = shufflevector <8 x i8> %in, <8 x i8> undef, <4 x i32> <i32 0, i32 2, i32 4, i32 6>
+  %tmp2 = shufflevector <8 x i8> %in, <8 x i8> undef, <4 x i32> <i32 1, i32 3, i32 5, i32 7>
+  %x = add <4 x i8> %tmp2, %tmp1
+  ret <4 x i8> %x
+}
+
+; Legalization promotes the <2 x i16> to <2 x i32>.
+define <2 x i16> @fromExtendingExtractVectorElt_i16(<4 x i16> %in) {
+;CHECK-LABEL: fromExtendingExtractVectorElt_i16:
 ;CHECK: vpaddl.s16
   %tmp1 = shufflevector <4 x i16> %in, <4 x i16> undef, <2 x i32> <i32 0, i32 2>
   %tmp2 = shufflevector <4 x i16> %in, <4 x i16> undef, <2 x i32> <i32 1, i32 3>
   %x = add <2 x i16> %tmp2, %tmp1
   ret <2 x i16> %x
 }
 
 declare <4 x i16> @llvm.arm.neon.vpaddls.v4i16.v8i8(<8 x i8>) nounwind readnone

test/CodeGen/ARM/vtrn.ll

 }
 
 define <2 x i32> @vtrni32(<2 x i32>* %A, <2 x i32>* %B) nounwind {
 ; CHECK-LABEL: vtrni32:
 ; CHECK:       @ BB#0:
 ; CHECK-NEXT:    vldr d16, [r1]
 ; CHECK-NEXT:    vldr d17, [r0]
 ; CHECK-NEXT:    vtrn.32 d17, d16
-; CHECK-NEXT:    vadd.i32 d16, d17, d16
+; CHECK-NEXT:    vmul.i32 d16, d17, d16
 ; CHECK-NEXT:    vmov r0, r1, d16
 ; CHECK-NEXT:    mov pc, lr
   %tmp1 = load <2 x i32>, <2 x i32>* %A
   %tmp2 = load <2 x i32>, <2 x i32>* %B
   %tmp3 = shufflevector <2 x i32> %tmp1, <2 x i32> %tmp2, <2 x i32> <i32 0, i32 2>
   %tmp4 = shufflevector <2 x i32> %tmp1, <2 x i32> %tmp2, <2 x i32> <i32 1, i32 3>
-        %tmp5 = add <2 x i32> %tmp3, %tmp4
+        %tmp5 = mul <2 x i32> %tmp3, %tmp4
   ret <2 x i32> %tmp5
 }
 
 define <4 x i32> @vtrni32_Qres(<2 x i32>* %A, <2 x i32>* %B) nounwind {
 ; CHECK-LABEL: vtrni32_Qres:
 ; CHECK:       @ BB#0:
 ; CHECK-NEXT:    vldr [[LDR1:d[0-9]+]], [r1]
 ; CHECK-NEXT:    vldr [[LDR0:d[0-9]+]], [r0]