[x86] minimal fix for horizontal binop matching for 256-bit vectors (PR40243)

[spatel]

Summary

After thinking about it more, the extra code in D56450 doesn't add much value if we can't remove the existing matcher. So this is a minimal alternative to that patch.

The existing code is safe/correct for 128-bit ops, but we need to adjust the outputs to account for undefs no matter what.

I still need to convince myself that the last section where we match 256-bit ops is always safe, but this does fix all of the known patterns from PR40243:
https://bugs.llvm.org/show_bug.cgi?id=40243

[spatel]

Abandoning - the original patch was already reviewed, so I went ahead and committed that + a similar fix as this patch implements.
That fixes all of the h-op miscompile problems that I am aware of.
I did eventually convince myself that the last section is safe based on the output of existing regression tests.

lib/Target/X86/X86ISelLowering.cpp

for (unsigned i = 0, e = NonConstIdx.size(); i != e; ++i) {
     unsigned InsertIdx = NonConstIdx[i];
     DstVec = DAG.getNode(ISD::INSERT_VECTOR_ELT, dl, VT, DstVec,
                          Op.getOperand(InsertIdx),
                          DAG.getIntPtrConstant(InsertIdx, dl));
   }
   return DstVec;
 }
 
-/// Return true if \p N implements a horizontal binop and return the
-/// operands for the horizontal binop into V0 and V1.
-///
 /// This is a helper function of LowerToHorizontalOp().
+/// This function checks that the build_vector \p N in input implements an x86
+/// 128-bit horizontal operation. It is also used to match 256-bit horizontal
+/// operations, but that requires the caller to adjust the results to produce
+/// a valid x86 256-bit horizontal instruction. In other words, if this returns
+/// true for a 256-bit input, then some extraction/insertion will be required to
+/// produce a horizontal instruction because the vector element indexes do not
+/// correspond to x86's 256-bit AVX ops.
+///
+/// Parameter \p Opcode defines the kind of horizontal operation to match.
 /// This function checks that the build_vector \p N in input implements a
 /// horizontal operation. Parameter \p Opcode defines the kind of horizontal
 /// operation to match.
 /// For example, if \p Opcode is equal to ISD::ADD, then this function
 /// checks if \p N implements a horizontal arithmetic add; if instead \p Opcode
 /// is equal to ISD::SUB, then this function checks if this is a horizontal
 /// arithmetic sub.
 ///
static SDValue LowerToHorizontalOp(const BuildVectorSDNode *BV,
 
   if ((VT == MVT::v8f32 || VT == MVT::v4f64)) {
     // Try to match an AVX horizontal add/sub of packed single/double
     // precision floating point values from 256-bit vectors.
     SDValue InVec2, InVec3;
     if (isHorizontalBinOp(BV, ISD::FADD, DAG, 0, Half, InVec0, InVec1) &&
         isHorizontalBinOp(BV, ISD::FADD, DAG, Half, NumElts, InVec2, InVec3) &&
         ((InVec0.isUndef() || InVec2.isUndef()) || InVec0 == InVec2) &&
-        ((InVec1.isUndef() || InVec3.isUndef()) || InVec1 == InVec3))
-      return DAG.getNode(X86ISD::FHADD, DL, VT, InVec0, InVec1);
-
+        ((InVec1.isUndef() || InVec3.isUndef()) || InVec1 == InVec3)) {
+      SDValue V0 = InVec0.isUndef() ? InVec2 : InVec0;
+      SDValue V1 = InVec1.isUndef() ? InVec3 : InVec1;
+      assert((!V0.isUndef() || !V1.isUndef()) && "Horizontal-op of undefs?");
+      return DAG.getNode(X86ISD::FHADD, DL, VT, V0, V1);
+    }
     if (isHorizontalBinOp(BV, ISD::FSUB, DAG, 0, Half, InVec0, InVec1) &&
         isHorizontalBinOp(BV, ISD::FSUB, DAG, Half, NumElts, InVec2, InVec3) &&
         ((InVec0.isUndef() || InVec2.isUndef()) || InVec0 == InVec2) &&
-        ((InVec1.isUndef() || InVec3.isUndef()) || InVec1 == InVec3))
-      return DAG.getNode(X86ISD::FHSUB, DL, VT, InVec0, InVec1);
+        ((InVec1.isUndef() || InVec3.isUndef()) || InVec1 == InVec3)) {
+      SDValue V0 = InVec0.isUndef() ? InVec2 : InVec0;
+      SDValue V1 = InVec1.isUndef() ? InVec3 : InVec1;
+      assert((!V0.isUndef() || !V1.isUndef()) && "Horizontal-op of undefs?");
+      return DAG.getNode(X86ISD::FHSUB, DL, VT, V0, V1);
+    }
   } else if (VT == MVT::v8i32 || VT == MVT::v16i16) {
     // Try to match an AVX2 horizontal add/sub of signed integers.
     SDValue InVec2, InVec3;
     unsigned X86Opcode;
     bool CanFold = true;
 
     if (isHorizontalBinOp(BV, ISD::ADD, DAG, 0, Half, InVec0, InVec1) &&
         isHorizontalBinOp(BV, ISD::ADD, DAG, Half, NumElts, InVec2, InVec3) &&
         ((InVec0.isUndef() || InVec2.isUndef()) || InVec0 == InVec2) &&
         ((InVec1.isUndef() || InVec3.isUndef()) || InVec1 == InVec3))
       X86Opcode = X86ISD::HADD;
     else if (isHorizontalBinOp(BV, ISD::SUB, DAG, 0, Half, InVec0, InVec1) &&
         isHorizontalBinOp(BV, ISD::SUB, DAG, Half, NumElts, InVec2, InVec3) &&
         ((InVec0.isUndef() || InVec2.isUndef()) || InVec0 == InVec2) &&
         ((InVec1.isUndef() || InVec3.isUndef()) || InVec1 == InVec3))
       X86Opcode = X86ISD::HSUB;
     else
       CanFold = false;
 
     if (CanFold) {
+      SDValue V0 = InVec0.isUndef() ? InVec2 : InVec0;
+      SDValue V1 = InVec1.isUndef() ? InVec3 : InVec1;
+      assert((!V0.isUndef() || !V1.isUndef()) && "Horizontal-op of undefs?");
+
       // Fold this build_vector into a single horizontal add/sub.
       // Do this only if the target has AVX2.
       if (Subtarget.hasAVX2())
-        return DAG.getNode(X86Opcode, DL, VT, InVec0, InVec1);
+        return DAG.getNode(X86Opcode, DL, VT, V0, V1);
 
       // Do not try to expand this build_vector into a pair of horizontal
       // add/sub if we can emit a pair of scalar add/sub.
       if (NumUndefsLO + 1 == Half || NumUndefsHI + 1 == Half)
         return SDValue();
 
       // Convert this build_vector into a pair of horizontal binop followed by
       // a concat vector.
       bool isUndefLO = NumUndefsLO == Half;
       bool isUndefHI = NumUndefsHI == Half;
-      return ExpandHorizontalBinOp(InVec0, InVec1, DL, DAG, X86Opcode, false,
+      return ExpandHorizontalBinOp(V0, V1, DL, DAG, X86Opcode, false,
                                    isUndefLO, isUndefHI);
     }
   }
 
   if ((VT == MVT::v8f32 || VT == MVT::v4f64 || VT == MVT::v8i32 ||
        VT == MVT::v16i16) && Subtarget.hasAVX()) {
+    // WARNING: Matching 256-bit horizontal ops here requires
+    // extracting/inserting from the source vectors.
+    // isHorizontalBinOp() does not match the element order for 256-bit AVX ops.
     unsigned X86Opcode;
     if (isHorizontalBinOp(BV, ISD::ADD, DAG, 0, NumElts, InVec0, InVec1))
       X86Opcode = X86ISD::HADD;
     else if (isHorizontalBinOp(BV, ISD::SUB, DAG, 0, NumElts, InVec0, InVec1))
       X86Opcode = X86ISD::HSUB;
     else if (isHorizontalBinOp(BV, ISD::FADD, DAG, 0, NumElts, InVec0, InVec1))
       X86Opcode = X86ISD::FHADD;
     else if (isHorizontalBinOp(BV, ISD::FSUB, DAG, 0, NumElts, InVec0, InVec1))

test/CodeGen/X86/haddsub-undef.ll

 ; SSE-FAST:       # %bb.0:
 ; SSE-FAST-NEXT:    haddps %xmm0, %xmm0
 ; SSE-FAST-NEXT:    haddps %xmm3, %xmm3
 ; SSE-FAST-NEXT:    movddup {{.*#+}} xmm1 = xmm3[0,0]
 ; SSE-FAST-NEXT:    retq
 ;
 ; AVX-LABEL: test11_undef:
 ; AVX:       # %bb.0:
-; AVX-NEXT:    vhaddps %ymm0, %ymm0, %ymm0
+; AVX-NEXT:    vhaddps %ymm1, %ymm0, %ymm0
 ; AVX-NEXT:    retq
   %vecext = extractelement <8 x float> %a, i32 0
   %vecext1 = extractelement <8 x float> %a, i32 1
   %add = fadd float %vecext, %vecext1
   %vecinit = insertelement <8 x float> undef, float %add, i32 0
   %vecext2 = extractelement <8 x float> %b, i32 4
   %vecext3 = extractelement <8 x float> %b, i32 5
   %add4 = fadd float %vecext2, %vecext3

 define <8 x float> @v16f32_inputs_v8f32_output_4567(<16 x float> %a, <16 x float> %b) {
 ; SSE-LABEL: v16f32_inputs_v8f32_output_4567:
 ; SSE:       # %bb.0:
 ; SSE-NEXT:    haddps %xmm5, %xmm1
 ; SSE-NEXT:    retq
 ;
 ; AVX1-SLOW-LABEL: v16f32_inputs_v8f32_output_4567:
 ; AVX1-SLOW:       # %bb.0:
-; AVX1-SLOW-NEXT:    vhaddps %ymm0, %ymm0, %ymm0
+; AVX1-SLOW-NEXT:    vhaddps %ymm2, %ymm0, %ymm0
 ; AVX1-SLOW-NEXT:    retq
 ;
 ; AVX1-FAST-LABEL: v16f32_inputs_v8f32_output_4567:
 ; AVX1-FAST:       # %bb.0:
-; AVX1-FAST-NEXT:    vhaddps %ymm0, %ymm0, %ymm0
+; AVX1-FAST-NEXT:    vhaddps %ymm2, %ymm0, %ymm0
 ; AVX1-FAST-NEXT:    retq
 ;
 ; AVX512-LABEL: v16f32_inputs_v8f32_output_4567:
 ; AVX512:       # %bb.0:
 ; AVX512-NEXT:    vextractf128 $1, %ymm0, %xmm0
 ; AVX512-NEXT:    vmovshdup {{.*#+}} xmm2 = xmm0[1,1,3,3]
 ; AVX512-NEXT:    vextractf128 $1, %ymm1, %xmm1
 ; AVX512-NEXT:    vpermilpd {{.*#+}} xmm3 = xmm1[1,0]

 define <8 x float> @PR40243(<8 x float> %a, <8 x float> %b) {
 ; SSE-LABEL: PR40243:
 ; SSE:       # %bb.0:
 ; SSE-NEXT:    haddps %xmm3, %xmm1
 ; SSE-NEXT:    retq
 ;
 ; AVX-LABEL: PR40243:
 ; AVX:       # %bb.0:
-; AVX-NEXT:    vhaddps %ymm0, %ymm0, %ymm0
+; AVX-NEXT:    vhaddps %ymm1, %ymm0, %ymm0
 ; AVX-NEXT:    retq
   %a4 = extractelement <8 x float> %a, i32 4
   %a5 = extractelement <8 x float> %a, i32 5
   %add4 = fadd float %a4, %a5
   %b6 = extractelement <8 x float> %b, i32 6
   %b7 = extractelement <8 x float> %b, i32 7
   %add7 = fadd float %b6, %b7
   %r4 = insertelement <8 x float> undef, float %add4, i32 4
   %r = insertelement <8 x float> %r4, float %add7, i32 7
   ret <8 x float> %r
 }
 

test/CodeGen/X86/phaddsub-undef.ll

 ; AVX1-NEXT:    vmovd %ecx, %xmm0
 ; AVX1-NEXT:    vmovd %edx, %xmm1
 ; AVX1-NEXT:    vpshufd {{.*#+}} xmm1 = xmm1[0,1,0,1]
 ; AVX1-NEXT:    vinsertf128 $1, %xmm1, %ymm0, %ymm0
 ; AVX1-NEXT:    retq
 ;
 ; AVX2-LABEL: test15_undef:
 ; AVX2:       # %bb.0:
-; AVX2-NEXT:    vphaddd %ymm0, %ymm0, %ymm0
+; AVX2-NEXT:    vphaddd %ymm1, %ymm0, %ymm0
 ; AVX2-NEXT:    retq
 ;
 ; AVX512-LABEL: test15_undef:
 ; AVX512:       # %bb.0:
-; AVX512-NEXT:    vphaddd %ymm0, %ymm0, %ymm0
+; AVX512-NEXT:    vphaddd %ymm1, %ymm0, %ymm0
 ; AVX512-NEXT:    retq
   %vecext = extractelement <8 x i32> %a, i32 0
   %vecext1 = extractelement <8 x i32> %a, i32 1
   %add = add i32 %vecext, %vecext1
   %vecinit = insertelement <8 x i32> undef, i32 %add, i32 0
   %vecext2 = extractelement <8 x i32> %b, i32 4
   %vecext3 = extractelement <8 x i32> %b, i32 5
   %add4 = add i32 %vecext2, %vecext3
   %vecinit5 = insertelement <8 x i32> %vecinit, i32 %add4, i32 6
   ret <8 x i32> %vecinit5
 }
 
 define <8 x i32> @PR40243_alt(<8 x i32> %a, <8 x i32> %b) {
 ; SSE-LABEL: PR40243_alt:
 ; SSE:       # %bb.0:
 ; SSE-NEXT:    phaddd %xmm3, %xmm1
 ; SSE-NEXT:    retq
 ;
 ; AVX1-LABEL: PR40243_alt:
 ; AVX1:       # %bb.0:
+; AVX1-NEXT:    vextractf128 $1, %ymm1, %xmm1
+; AVX1-NEXT:    vextractf128 $1, %ymm0, %xmm0
+; AVX1-NEXT:    vphaddd %xmm1, %xmm0, %xmm0
+; AVX1-NEXT:    vinsertf128 $1, %xmm0, %ymm0, %ymm0
 ; AVX1-NEXT:    retq
 ;
 ; AVX2-LABEL: PR40243_alt:
 ; AVX2:       # %bb.0:
-; AVX2-NEXT:    vphaddd %ymm0, %ymm0, %ymm0
+; AVX2-NEXT:    vphaddd %ymm1, %ymm0, %ymm0
 ; AVX2-NEXT:    retq
 ;
 ; AVX512-LABEL: PR40243_alt:
 ; AVX512:       # %bb.0:
-; AVX512-NEXT:    vphaddd %ymm0, %ymm0, %ymm0
+; AVX512-NEXT:    vphaddd %ymm1, %ymm0, %ymm0
 ; AVX512-NEXT:    retq
   %a4 = extractelement <8 x i32> %a, i32 4
   %a5 = extractelement <8 x i32> %a, i32 5
   %add4 = add i32 %a4, %a5
   %b6 = extractelement <8 x i32> %b, i32 6
   %b7 = extractelement <8 x i32> %b, i32 7
   %add7 = add i32 %b6, %b7
   %r4 = insertelement <8 x i32> undef, i32 %add4, i32 4