Don't crash when we see unallocatable registers in clobbers

[george.burgess.iv]

Summary

This is part 1 of addressing the feedback from D38479. Apologies in advanced if these miss anything obvious; I've spent very little time working with backend code. :)

This also fixes the immediate issue raised by the test-case for https://bugs.llvm.org/show_bug.cgi?id=30792, but the moment you try to use FP/vector instructions with the integrated assembler, you'll get complaints. Since the spirit of -mgeneral-regs-only is "the assembler can use non-general registers," we'll probably need to address that, as well.

[george.burgess.iv]

Simplify test-case

(Sorry for the noise; I was trying to swap to uploading this diff first, but rather than hitting the "swap tab" shortcut to migrate the title/summary/etc. over, I hit cmd+enter. Doing that apparently submits the diff for review.)

[efriedma]

lib/CodeGen/SelectionDAG/LegalizeTypes.h

97

Might as well just allow all ISD::Register nodes? The type of an ISD::Register isn't really significant anyway.

[george.burgess.iv]

Addressed all feedback. Thanks!

[efriedma]

LGTM, with one minor adjustment.

lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp

948

Somehow you lost a comment from the original code. Also, the if statement around the assertion is a little weird. Maybe something like the following?

assert((Regs[I] != SP ||
        DAG.getMachineFunction().getFrameInfo().hasOpaqueSPAdjustment()) &&
       "If we clobbered the stack pointer, MFI should know about it.")

[george.burgess.iv]

Thanks again!

lib/CodeGen/SelectionDAG/LegalizeDAG.cpp

getStrictFPOpcodeAction(const TargetLowering &TLI, unsigned Opcode, EVT VT) {
   // We don't currently handle Custom or Promote for strict FP pseudo-ops.
   // For now, we just expand for those cases.
   if (Action != TargetLowering::Legal)
     Action = TargetLowering::Expand;
 
   return Action;
 }
 
+static bool illegalNodeIsAcceptable(const SDNode *Node) {
+  // Allow illegal target nodes and illegal registers only referenced by
+  // inline asm.
+  if (Node->getOpcode() == ISD::Register) {
+    return all_of(Node->uses(), [](const SDNode *O) {
+      return O->getOpcode() == ISD::INLINEASM;
+    });
+  }
+  return Node->getOpcode() == ISD::TargetConstant;
+}
+
 /// Return a legal replacement for the given operation, with all legal operands.
 void SelectionDAGLegalize::LegalizeOp(SDNode *Node) {
   DEBUG(dbgs() << "\nLegalizing: "; Node->dump(&DAG));
 
-  if (Node->getOpcode() == ISD::TargetConstant) // Allow illegal target nodes.
+  if (illegalNodeIsAcceptable(Node))
     return;
 
 #ifndef NDEBUG
   for (unsigned i = 0, e = Node->getNumValues(); i != e; ++i)
     assert((TLI.getTypeAction(*DAG.getContext(), Node->getValueType(i)) ==
               TargetLowering::TypeLegal ||
             TLI.isTypeLegal(Node->getValueType(i))) &&
            "Unexpected illegal type!");
 
   for (const SDValue &Op : Node->op_values())
     assert((TLI.getTypeAction(*DAG.getContext(), Op.getValueType()) ==
               TargetLowering::TypeLegal ||
             TLI.isTypeLegal(Op.getValueType()) ||
-            Op.getOpcode() == ISD::TargetConstant) &&
+            illegalNodeIsAcceptable(Op.getNode())) &&
             "Unexpected illegal type!");
 #endif
 
   // Figure out the correct action; the way to query this varies by opcode
   TargetLowering::LegalizeAction Action = TargetLowering::Legal;
   bool SimpleFinishLegalizing = true;
   switch (Node->getOpcode()) {
   case ISD::INTRINSIC_W_CHAIN:

lib/CodeGen/SelectionDAG/LegalizeTypes.h

private:
   }
 
   EVT getSetCCResultType(EVT VT) const {
     return TLI.getSetCCResultType(DAG.getDataLayout(), *DAG.getContext(), VT);
   }
 
   /// Pretend all of this node's results are legal.
   bool IgnoreNodeResults(SDNode *N) const {
+    if (N->getOpcode() == ISD::Register) {
+      // Assume that registers only used by inline asm were vetted properly
+      // when they were parsed.
+      return all_of(N->uses(), [](const SDNode *O) {
+        return O->getOpcode() == ISD::INLINEASM;
+      });
+    }
     return N->getOpcode() == ISD::TargetConstant;
   }
 
   /// For integer nodes that are below legal width, this map indicates what
   /// promoted value to use.
   SmallDenseMap<SDValue, SDValue, 8> PromotedIntegers;
 
   /// For integer nodes that need to be expanded this map indicates which

lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp

else if (!Regs.empty() &&
     const MachineRegisterInfo &MRI = DAG.getMachineFunction().getRegInfo();
     const TargetRegisterClass *RC = MRI.getRegClass(Regs.front());
     Flag = InlineAsm::getFlagWordForRegClass(Flag, RC->getID());
   }
 
   SDValue Res = DAG.getTargetConstant(Flag, dl, MVT::i32);
   Ops.push_back(Res);
 
+  if (Code == InlineAsm::Kind_Clobber) {
+    // Clobbers should always have a 1:1 mapping with registers, and may
+    // reference registers that have illegal (e.g. vector) types. Hence, we
+    // shouldn't try to apply any sort of splitting logic to them.
+    assert(Regs.size() == RegVTs.size() && Regs.size() == ValueVTs.size() &&
+           "No 1:1 mapping from clobbers to regs?");
     unsigned SP = TLI.getStackPointerRegisterToSaveRestore();
+    for (unsigned I = 0, E = ValueVTs.size(); I != E; ++I) {
+      Ops.push_back(DAG.getRegister(Regs[I], RegVTs[I]));
+      if (Regs[I] == SP) {
+        assert(DAG.getMachineFunction().getFrameInfo().hasOpaqueSPAdjustment());
+      }
+    }
+    return;
+  }
+
   for (unsigned Value = 0, Reg = 0, e = ValueVTs.size(); Value != e; ++Value) {
     unsigned NumRegs = TLI.getNumRegisters(*DAG.getContext(), ValueVTs[Value]);
     MVT RegisterVT = RegVTs[Value];
     for (unsigned i = 0; i != NumRegs; ++i) {
       assert(Reg < Regs.size() && "Mismatch in # registers expected");
       unsigned TheReg = Regs[Reg++];
       Ops.push_back(DAG.getRegister(TheReg, RegisterVT));
-
-      if (TheReg == SP && Code == InlineAsm::Kind_Clobber) {
-        // If we clobbered the stack pointer, MFI should know about it.
-        assert(DAG.getMachineFunction().getFrameInfo().hasOpaqueSPAdjustment());
-      }
     }
   }
 }
 
 void SelectionDAGBuilder::init(GCFunctionInfo *gfi, AliasAnalysis *aa,
                                const TargetLibraryInfo *li) {
   AA = aa;
   GFI = gfi;

test/CodeGen/AArch64/no-fp-asm-clobbers-crash.ll

+; RUN: not llc < %s | FileCheck %s
+;
+; Be sure that we ignore clobbers of unallocatable registers, rather than
+; crashing.
+
+target datalayout = "e-m:e-i8:8:32-i16:16:32-i64:64-i128:128-n32:64-S128"
+target triple = "aarch64"
+
+; CHECK-LABEL: foo:
+; Should make the asm parser unhappy.
+; CHECK-NOT: add v0, v1
+; CHECK: ret
+define void @foo() #0 {
+entry:
+  call void asm sideeffect "add v0.2d, v1.2d, v2.2d", "~{v0},~{v1},~{v2}"()
+  ret void
+}
+
+; CHECK-LABEL: bar:
+; Should make the asm parser unhappy.
+; CHECK-NOT: fmov s1, s0
+; CHECK: ret
+define void @bar() #0 {
+entry:
+  call void asm sideeffect "fmov s1, s0", "~{s0},~{s1}"()
+  ret void
+}
+
+attributes #0 = { nounwind "target-features"="-crypto,-fp-armv8,-neon" }