X86: Do not select X86 custom vector nodes if operand types don't match

[MatzeB]

Summary

X86ISD::ADDSUB, X86ISD::(F)HADD, X86ISD::(F)HSUB should not be selected
if the operand types do not match the result type because vector type
legalization cannot deal with this for custom nodes.

Testcase X86ISD::ADDSUB is attached. I could not create a testcase for
the FHADD/FHSUB cases because of: https://llvm.org/bugs/show_bug.cgi?id=23296

[ab]

For the missing testcase, one hack you can use to force an additional DAGCombine round is to introduce illegal vector ops (so there's another vector ops legalization phase, with the associated combining round), since IIRC the BUILD_VECTOR->SHUFP lowering happens in the final ops legalization phase, after vector ops legalization. See r235243, where I used ctpop. That's icky though, and shows that our AddUsersToWorklist tricks aren't enough (but that's a discussion for another day ;)

lib/Target/X86/X86ISelLowering.cpp

5272–5285 ↗

(On Diff #24040)

Should you put the checks outside the if/else? Something like

if (V0.getVT() != VT || V1.getVT != VT)
  return false;

Not sure it's better.

[andreadb]

Hi Matthias,
the patch looks good to me. I only added a very minor comment in the test (see below).

As a side note: I think that the major problem here is that the logic that matches horizontal add/sub operations is run as part of a target specific combine on BUILD_VECTOR nodes.
This is sub-optimal and may lead to a premature selection of target specific nodes.
As a long term fix, in bug 23296 I suggested to see if it is possible/reasonable to move all the horizontal add/sub selection logic from performBUILD_VECTORCombine in (a function called by) LowerBUILD_VECTOR.

Thanks,
Andrea

test/CodeGen/X86/sse3-avx-addsub-2.ll

318 ↗

(On Diff #24040)

You can remove the '#0'. That attribute set is not defined.

[MatzeB]

Thanks for the reviews. I decided against pulling out the getVT() == VT checks, as then I would need extra checks to guard against Undef which doesn't make the code nicer IMO.

Changing horizontal add/sub and ADDSUB matching to not be a DAGCombine but a lowering makes perfect sense, I'll try to change the code to that.

llvm/trunk/lib/Target/X86/X86ISelLowering.cpp

CanFold = (Op0.getOpcode() == ISD::EXTRACT_VECTOR_ELT &&
         isa<ConstantSDNode>(Op1.getOperand(1)));
     if (!CanFold)
       break;
 
     unsigned I0 = cast<ConstantSDNode>(Op0.getOperand(1))->getZExtValue();
     unsigned I1 = cast<ConstantSDNode>(Op1.getOperand(1))->getZExtValue();
 
     if (i * 2 < NumElts) {
-      if (V0.getOpcode() == ISD::UNDEF)
+      if (V0.getOpcode() == ISD::UNDEF) {
         V0 = Op0.getOperand(0);
+        if (V0.getValueType() != VT)
+          return false;
+      }
     } else {
-      if (V1.getOpcode() == ISD::UNDEF)
+      if (V1.getOpcode() == ISD::UNDEF) {
         V1 = Op0.getOperand(0);
+        if (V1.getValueType() != VT)
+          return false;
+      }
       if (i * 2 == NumElts)
         ExpectedVExtractIdx = BaseIdx;
     }
 
     SDValue Expected = (i * 2 < NumElts) ? V0 : V1;
     if (I0 == ExpectedVExtractIdx)
       CanFold = I1 == I0 + 1 && Op0.getOperand(0) == Expected;
     else if (IsCommutable && I1 == ExpectedVExtractIdx) {
for (unsigned i = 0, e = NumElts; i != e; ++i) {
 
     // We found a valid add/sub node. Update the information accordingly.
     if (i & 1)
       AddFound = true;
     else
       SubFound = true;
 
     // Update InVec0 and InVec1.
-    if (InVec0.getOpcode() == ISD::UNDEF)
+    if (InVec0.getOpcode() == ISD::UNDEF) {
       InVec0 = Op0.getOperand(0);
-    if (InVec1.getOpcode() == ISD::UNDEF)
+      if (InVec0.getValueType() != VT)
+        return SDValue();
+    }
+    if (InVec1.getOpcode() == ISD::UNDEF) {
       InVec1 = Op1.getOperand(0);
+      if (InVec1.getValueType() != VT)
+        return SDValue();
+    }
 
     // Make sure that operands in input to each add/sub node always
     // come from a same pair of vectors.
     if (InVec0 != Op0.getOperand(0)) {
       if (ExpectedOpcode == ISD::FSUB)
         return SDValue();
 
       // FADD is commutable. Try to commute the operands

llvm/trunk/test/CodeGen/X86/sse3-avx-addsub-2.ll

define <4 x float> @test16(<4 x float> %A, <4 x float> %B) {
   %vecinsert3 = insertelement <4 x float> %vecinsert2, float %sub, i32 0
   %vecinsert4 = insertelement <4 x float> %vecinsert3, float %sub2, i32 2
   ret <4 x float> %vecinsert4
 }
 ; CHECK-LABEL: test16
 ; CHECK-NOT: addsubps
 ; CHECK: ret
 
-
+define <2 x float> @test_v2f32(<2 x float> %v0, <2 x float> %v1) {
+  %v2 = extractelement <2 x float> %v0, i32 0
+  %v3 = extractelement <2 x float> %v1, i32 0
+  %v4 = extractelement <2 x float> %v0, i32 1
+  %v5 = extractelement <2 x float> %v1, i32 1
+  %sub = fsub float %v2, %v3
+  %add = fadd float %v5, %v4
+  %res0 = insertelement <2 x float> undef, float %sub, i32 0
+  %res1 = insertelement <2 x float> %res0, float %add, i32 1
+  ret <2 x float> %res1
+}
+; CHECK-LABEL: test_v2f32
+; CHECK: addsubps %xmm1, %xmm0
+; CHECK-NEXT: retq