[AArch64][DAGCombine] Fix a bug in performBuildVectorCombine where it could produce an invalid EXTRACT_SUBVECTOR

[mnadeem]

Summary

EXTRACT_SUBVECTOR requires that Idx be a constant multiple of ResultType's
known minimum vector length.

Something like this will produce an invalid extract_subvector:

t1: v4i16 = .....
t2: i32 = extract_vector_elt t1, Constant:i64<1>
t3: i32 = extract_vector_elt t1, Constant:i64<2>
t4: v2i32 = BUILD_VECTOR t2, t3
// produces
t5: v2i32 = extract_subvector t...., Constant:i64<1>

[fhahn]

Something like this will produce an invalid extract_subvector:

Could you attach a test case?

[paulwalker-arm]

A test would be nice but otherwise the patch looks good. FYI: I didn't realise this requirement existed for fixed length vectors and it seems SelectionDAG::getNode() is missing the relevant asserts. However, when I add them I'm seeing only non-aarch64 failures, so I'm not going to open that can of worms just yet.

[mnadeem]

Added a reduced test case to check that we don't crash.

[mnadeem]

Should I go ahead and merge if the test looks okay?

llvm/lib/Target/AArch64/AArch64ISelLowering.cpp

static SDValue performVectorAddSubExtCombine(SDNode *N, SelectionDAG &DAG) {
   }
   return SDValue();
 }
 
 static SDValue performBuildVectorCombine(SDNode *N,
                                          TargetLowering::DAGCombinerInfo &DCI,
                                          SelectionDAG &DAG) {
   SDLoc DL(N);
+  EVT VT = N->getValueType(0);
 
   // A build vector of two extracted elements is equivalent to an
   // extract subvector where the inner vector is any-extended to the
   // extract_vector_elt VT.
   //    (build_vector (extract_elt_iXX_to_i32 vec Idx+0)
   //                  (extract_elt_iXX_to_i32 vec Idx+1))
   // => (extract_subvector (anyext_iXX_to_i32 vec) Idx)
 
   // For now, only consider the v2i32 case, which arises as a result of
   // legalization.
-  if (N->getValueType(0) != MVT::v2i32)
+  if (VT != MVT::v2i32)
     return SDValue();
 
   SDValue Elt0 = N->getOperand(0), Elt1 = N->getOperand(1);
   // Reminder, EXTRACT_VECTOR_ELT has the effect of any-extending to its VT.
   if (Elt0->getOpcode() == ISD::EXTRACT_VECTOR_ELT &&
       Elt1->getOpcode() == ISD::EXTRACT_VECTOR_ELT &&
       // Constant index.
       isa<ConstantSDNode>(Elt0->getOperand(1)) &&
       isa<ConstantSDNode>(Elt1->getOperand(1)) &&
       // Both EXTRACT_VECTOR_ELT from same vector...
       Elt0->getOperand(0) == Elt1->getOperand(0) &&
       // ... and contiguous. First element's index +1 == second element's index.
-      Elt0->getConstantOperandVal(1) + 1 == Elt1->getConstantOperandVal(1)) {
+      Elt0->getConstantOperandVal(1) + 1 == Elt1->getConstantOperandVal(1) &&
+      // EXTRACT_SUBVECTOR requires that Idx be a constant multiple of
+      // ResultType's known minimum vector length.
+      Elt0->getConstantOperandVal(1) % VT.getVectorMinNumElements() == 0) {
     SDValue VecToExtend = Elt0->getOperand(0);
     EVT ExtVT = VecToExtend.getValueType().changeVectorElementType(MVT::i32);
     if (!DAG.getTargetLoweringInfo().isTypeLegal(ExtVT))
       return SDValue();
 
     SDValue SubvectorIdx = DAG.getVectorIdxConstant(Elt0->getConstantOperandVal(1), DL);
 
     SDValue Ext = DAG.getNode(ISD::ANY_EXTEND, DL, ExtVT, VecToExtend);