Fix in SelectionDAG::getNode() to not produce illegal BUILD_VECTOR operands

[jonpa]

Summary

Continued from: https://bugs.llvm.org//show_bug.cgi?id=32422

About the assert in LegalizeOp() that triggered:

llvm/llvm-dev/lib/CodeGen/SelectionDAG/LegalizeDAG.cpp:921: void {anonymous}::SelectionDAGLegalize::LegalizeOp(llvm::SDNode*): Assertion `(TLI.getTypeAction(*DAG.getContext(), Op.getValueType()) == TargetLowering::TypeLegal || TLI.isTypeLegal(Op.getValueType()) || Op.getOpcode() == ISD::TargetConstant) && "Unexpected illegal type!"' failed.

I wonder why both getTypeAction() and isTypeLegal() are called here - shouldn't they be equivalent?

Follow up question is then If the patch looks reasonable: is it enough with just isTypeLegal()?

I also removed the check
if (Ops.size() == LegalVT.getVectorNumElements())

since it seems redundant with
if (ISD::isBuildVectorOfConstantSDNodes(N1.getNode()))

[jonpa]

Test case (which I did not manage to reduce further...)

[niravd]

lib/CodeGen/SelectionDAG/SelectionDAG.cpp

4143

It doesn't seems like we should have to consider if the type is legal. VT should never have been used.

We should be able to use Op.getValueType().getScalarType() wherever LegalScalarVT is used.

4149

This removal of the redundant check seems reasonable and should probably be done as a separate NFC cleanup.

[efriedma]

I wonder why both getTypeAction() and isTypeLegal() are called here - shouldn't they be equivalent?

See https://reviews.llvm.org/rL254653 .

lib/CodeGen/SelectionDAG/SelectionDAG.cpp

4138

The getScalarType() here is redundant: the type of N1C must be an integer type.

[jonpa]

lib/CodeGen/SelectionDAG/SelectionDAG.cpp

4143

Changed so the BUILD_VECTOR operand type is unchanged in the new BUILD_VECTOR. I thought first there might be a point in having a legal smaller operand type, but at second thought there seems to be no point as BUILD_VECTOR will truncate and the smaller constant operands should get put into the smaller vector still.

Also removed the call to zextOrTrunc() since we keep the same VT for Op.

4149

OK, will do a separate NFC commit for the redundant check.

[niravd]

LGTM.

[jonpa]

Thanks!

r299540

lib/CodeGen/SelectionDAG/SelectionDAG.cpp

assert(EVT.isVector() == VT.isVector() &&
            "SIGN_EXTEND_INREG type should be vector iff the operand "
            "type is vector!");
     assert((!EVT.isVector() ||
             EVT.getVectorNumElements() == VT.getVectorNumElements()) &&
            "Vector element counts must match in SIGN_EXTEND_INREG");
     assert(EVT.bitsLE(VT) && "Not extending!");
     if (EVT == VT) return N1;  // Not actually extending
 
-    auto SignExtendInReg = [&](APInt Val) {
+    auto SignExtendInReg = [&](APInt Val, llvm::EVT ConstantVT) {
       unsigned FromBits = EVT.getScalarSizeInBits();
       Val <<= Val.getBitWidth() - FromBits;
       Val = Val.ashr(Val.getBitWidth() - FromBits);
-      return getConstant(Val, DL, VT.getScalarType());
+      return getConstant(Val, DL, ConstantVT);
     };
 
     if (N1C) {
       const APInt &Val = N1C->getAPIntValue();
-      return SignExtendInReg(Val);
+      return SignExtendInReg(Val, VT);
     }
     if (ISD::isBuildVectorOfConstantSDNodes(N1.getNode())) {
       SmallVector<SDValue, 8> Ops;
+      llvm::EVT OpVT = N1.getOperand(0).getValueType();
       for (int i = 0, e = VT.getVectorNumElements(); i != e; ++i) {
         SDValue Op = N1.getOperand(i);
         if (Op.isUndef()) {
-          Ops.push_back(getUNDEF(VT.getScalarType()));
+          Ops.push_back(getUNDEF(OpVT));
           continue;
         }
         if (ConstantSDNode *C = dyn_cast<ConstantSDNode>(Op)) {
           APInt Val = C->getAPIntValue();
-          Val = Val.zextOrTrunc(VT.getScalarSizeInBits());
-          Ops.push_back(SignExtendInReg(Val));
+          Ops.push_back(SignExtendInReg(Val, OpVT));
           continue;
         }
         break;
       }
       if (Ops.size() == VT.getVectorNumElements())
         return getBuildVector(VT, DL, Ops);
     }
     break;

test/CodeGen/SystemZ/DAGCombiner_illegal_BUILD_VECTOR.ll

+; RUN: llc < %s -mtriple=s390x-linux-gnu -mcpu=z13 | FileCheck %s
+;
+; Check that DAGCombiner does not crash after producing an illegal
+; BUILD_VECTOR node.
+
+
+define void @pr32422() {
+; CHECK:        cdbr    %f0, %f0
+; CHECK:        jo      .LBB0_1
+
+BB:
+  %I = insertelement <8 x i8> zeroinitializer, i8 -95, i32 3
+  %I8 = insertelement <8 x i8> zeroinitializer, i8 -119, i32 2
+  %FC = uitofp <8 x i8> %I8 to <8 x float>
+  %Cmp18 = fcmp uno <8 x float> zeroinitializer, %FC
+  %I22 = insertelement <8 x i1> %Cmp18, i1 true, i32 5
+  br label %CF
+
+CF:                                               ; preds = %CF, %BB
+  %Cmp40 = fcmp uno double 0xC663C682E9619F00, undef
+  br i1 %Cmp40, label %CF, label %CF353
+
+CF353:                                            ; preds = %CF
+  %E195 = extractelement <8 x i1> %I22, i32 4
+  ret void
+}