[SelectionDAG] Add stub implementation of ReplaceInsertSubVectorResults

[joechrisellis]

Summary

This is required to prevent hitting an llvm_unreachable when
ReplaceNodeResults is called on an INSERT_SUBVECTOR node. At the moment,
this implementation simply delegates to common code. It is likely that
in the future we will want to change this, but for now, the stub
implementation is sufficient to prevent crashing.

This patch is part of a wider piece of work to allow INSERT_SUBVECTOR to
be lowered when inserting into a smaller-than-legal scalable type. Any
code examples that hit the bad codepath will hit other crashes further
down the line, so this patch does not include tests. Tests which guard
against the crashing behaviour will be added in a later patch.

[paulwalker-arm]

llvm/lib/Target/AArch64/AArch64ISelLowering.cpp

16728–16730

I'm not saying this is not the correct solution but you typically opt into having custom handling via ReplaceNodeResults so perhaps we've requested (via setOperationAction()) something we didn't want?

[joechrisellis]

llvm/lib/Target/AArch64/AArch64ISelLowering.cpp

16728–16730

That's a good point. I reckon we're hitting the unreachable because of the following code block in AArch64ISelLowering.cpp:

// Illegal unpacked integer vector types.
for (auto VT : {MVT::nxv8i8, MVT::nxv4i16, MVT::nxv2i32}) {
  setOperationAction(ISD::EXTRACT_SUBVECTOR, VT, Custom);
  setOperationAction(ISD::INSERT_SUBVECTOR, VT, Custom); // <--
}

I haven't done much digging, but I think that's important. Reverting this patch locally, then removing that line fixes the issues that this patch resolves, but means that the following tests fail:

llvm/test/CodeGen/AArch64/sve-masked-gather-legalize.ll
llvm/test/CodeGen/AArch64/sve-split-fcvt.ll
llvm/test/CodeGen/AArch64/sve-split-trunc.ll

with Do not know how to promote this operator's operand!. This makes me think we probably need to keep those lines.

[paulwalker-arm]

llvm/lib/Target/AArch64/AArch64ISelLowering.cpp

16728–16730

That makes sense, thanks for checking @joechrisellis .

In which I'll just say there doesn't seem much value in creating an empty function, but rather can just add a comment here along the lines of "We've requested custom lowering but rely on common code for result type legalisation." and return directly.

Given this'll make this a two line change without any tests I think you may as well just add the code to D102766.

[joechrisellis]

@paulwalker-arm ACK -- will fold this into D102766. 🙂

[joechrisellis]

This change will be folded into D102766.

llvm/lib/Target/AArch64/AArch64ISelLowering.h

bool getPostIndexedAddressParts(SDNode *N, SDNode *Op, SDValue &Base,
                                   SDValue &Offset, ISD::MemIndexedMode &AM,
                                   SelectionDAG &DAG) const override;
 
   void ReplaceNodeResults(SDNode *N, SmallVectorImpl<SDValue> &Results,
                           SelectionDAG &DAG) const override;
   void ReplaceExtractSubVectorResults(SDNode *N,
                                       SmallVectorImpl<SDValue> &Results,
                                       SelectionDAG &DAG) const;
+  void ReplaceInsertSubVectorResults(SDNode *N,
+                                     SmallVectorImpl<SDValue> &Results,
+                                     SelectionDAG &DAG) const;
 
   bool shouldNormalizeToSelectSequence(LLVMContext &, EVT) const override;
 
   void finalizeLowering(MachineFunction &MF) const override;
 
   bool shouldLocalize(const MachineInstr &MI,
                       const TargetTransformInfo *TTI) const override;
 

llvm/lib/Target/AArch64/AArch64ISelLowering.cpp

void AArch64TargetLowering::ReplaceExtractSubVectorResults(
 
   unsigned Opcode = (Index == 0) ? AArch64ISD::UUNPKLO : AArch64ISD::UUNPKHI;
   EVT ExtendedHalfVT = VT.widenIntegerVectorElementType(*DAG.getContext());
 
   SDValue Half = DAG.getNode(Opcode, DL, ExtendedHalfVT, N->getOperand(0));
   Results.push_back(DAG.getNode(ISD::TRUNCATE, DL, VT, Half));
 }
 
+void AArch64TargetLowering::ReplaceInsertSubVectorResults(
+    SDNode *N, SmallVectorImpl<SDValue> &Results, SelectionDAG &DAG) const {
+  // TODO: Currently delegating to common code. This might want changing in the
+  //       future.
+  return;
+}
+
 // Create an even/odd pair of X registers holding integer value V.
 static SDValue createGPRPairNode(SelectionDAG &DAG, SDValue V) {
   SDLoc dl(V.getNode());
   SDValue VLo = DAG.getAnyExtOrTrunc(V, dl, MVT::i64);
   SDValue VHi = DAG.getAnyExtOrTrunc(
       DAG.getNode(ISD::SRL, dl, MVT::i128, V, DAG.getConstant(64, dl, MVT::i64)),
       dl, MVT::i64);
   if (DAG.getDataLayout().isBigEndian())
case ISD::LOAD: {
     SDValue Pair = DAG.getNode(ISD::BUILD_PAIR, SDLoc(N), MVT::i128,
                                Result.getValue(0), Result.getValue(1));
     Results.append({Pair, Result.getValue(2) /* Chain */});
     return;
   }
   case ISD::EXTRACT_SUBVECTOR:
     ReplaceExtractSubVectorResults(N, Results, DAG);
     return;
+  case ISD::INSERT_SUBVECTOR:
+    ReplaceInsertSubVectorResults(N, Results, DAG);
+    return;
   case ISD::INTRINSIC_WO_CHAIN: {
     EVT VT = N->getValueType(0);
     assert((VT == MVT::i8 || VT == MVT::i16) &&
            "custom lowering for unexpected type");
 
     ConstantSDNode *CN = cast<ConstantSDNode>(N->getOperand(0));
     Intrinsic::ID IntID = static_cast<Intrinsic::ID>(CN->getZExtValue());
     switch (IntID) {