Handling of TRAP during isel

[jonpa]

Summary

The one thing that keeps the SystemZ backend from enabling expensive checks, is the issue with the trap instruction. See https://bugs.llvm.org/show_bug.cgi?id=33047.

Current discussion is:

• It looks like targets are split over whether they consider their "trap" a terminator; x86, AArch64, and NVPTX don't, but ARM, MIPS, PPC, and SystemZ do. We should probably try to be consistent here.

• If the isTerminator is flag is used, how should isel handle instructions after it? Note that this is only an issue at -O0.

I did a quick experiment to see what would happen if I just added isTerminator on all targets trap instructions, and then handled TRAP during selection DAG building. This however caused a lot of regression test failures, so this is not a working patch right now.

[efriedma]

Needs testcase. (-verify-machineinstrs will run the verifier even if it's off by default for your target.)

Some targets special-case Intrinsic::trap in FastISel; need separate testcases for that (and maybe fixes).

I'm not sure I agree with the approach; just because it doesn't return doesn't mean you have to mark it a terminator. A call to the C library function abort() is also a "terminator", but we don't treat it as one because it's just simpler to treat it like a regular call.

[jonpa]

Some targets special-case Intrinsic::trap in FastISel; need separate testcases for that (and maybe fixes).

I'm not sure I agree with the approach; just because it doesn't return doesn't mean you have to mark it a terminator. A call to the C library function abort() is also a "terminator", but we don't treat it as one because it's just simpler to treat it like a regular call.

Does that mean that it is enough to have the hasCtrlDep / hasSideEffects flag, so that it will never be removed rescheduled / removed? What happens in this test case on SystemZ is that any returning argument COPYs will be incorrectly be placed before it, but maybe that doesn't matter in the case of a trapping instruction (?).

Is there any opinion from any other target maintainer on this?

[jonpa]

Due to the many test failures on the other targets, it seems much simpler to remove the barrier and terminator flags on the trap instructions. This seems to work fine for SystemZ without any code change on benchmarks.

lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp

SDValue SelectionDAGBuilder::getControlRoot() {
   Root = DAG.getNode(ISD::TokenFactor, getCurSDLoc(), MVT::Other,
                      PendingExports);
   PendingExports.clear();
   DAG.setRoot(Root);
   return Root;
 }
 
 void SelectionDAGBuilder::visit(const Instruction &I) {
+  // Don't create any nodes after a TRAP.
+  if (getControlRoot()->getOpcode() == ISD::TRAP)
+    return;
+
   // Set up outgoing PHI node register values before emitting the terminator.
   if (isa<TerminatorInst>(&I)) {
     HandlePHINodesInSuccessorBlocks(I.getParent());
   }
 
   // Increase the SDNodeOrder if dealing with a non-debug instruction.
   if (!isa<DbgInfoIntrinsic>(I))
     ++SDNodeOrder;

lib/Target/AArch64/AArch64InstrInfo.td

 let isCall = 1, Defs = [LR], Uses = [SP] in {
 def BL : CallImm<1, "bl", [(AArch64call tglobaladdr:$addr)]>;
 } // isCall
 def : Pat<(AArch64call texternalsym:$func), (BL texternalsym:$func)>;
 
 //===----------------------------------------------------------------------===//
 // Exception generation instructions.
 //===----------------------------------------------------------------------===//
+let isTerminator = 1 in  // Consider trap a terminator
   def BRK   : ExceptionGeneration<0b001, 0b00, "brk">;
 def DCPS1 : ExceptionGeneration<0b101, 0b01, "dcps1">;
 def DCPS2 : ExceptionGeneration<0b101, 0b10, "dcps2">;
 def DCPS3 : ExceptionGeneration<0b101, 0b11, "dcps3">;
 def HLT   : ExceptionGeneration<0b010, 0b00, "hlt">;
 def HVC   : ExceptionGeneration<0b000, 0b10, "hvc">;
 def SMC   : ExceptionGeneration<0b000, 0b11, "smc">;
 def SVC   : ExceptionGeneration<0b000, 0b01, "svc">;
 

lib/Target/Mips/Mips16InstrInfo.td

 def BnezRxImmX16: FEXT_RI16_B_ins<0b00101, "bnez", IIM16Alu>, cbranch16;
 
 
 //
 //Format: BREAK immediate
 // Purpose: Breakpoint
 // To cause a Breakpoint exception.
 
+let isTerminator = 1 in
 def Break16: FRRBreakNull16_ins<"break 0", IIM16Alu>;
 //
 // Format: BTEQZ offset MIPS16e
 // Purpose: Branch on T Equal to Zero (Extended)
 // To test special register T then do a PC-relative conditional branch.
 //
 def Bteqz16: FI816_ins<0b000, "bteqz", IIM16Alu>, cbranch16 {
   let Uses = [T8];

lib/Target/Mips/MipsInstrInfo.td

 def TLTI : MMRel, TEQI_FT<"tlti", GPR32Opnd, II_TLTI>, TEQI_FM<0xa>,
            ISA_MIPS2_NOT_32R6_64R6;
 def TTLTIU : MMRel, TEQI_FT<"tltiu", GPR32Opnd, II_TTLTIU>, TEQI_FM<0xb>,
            ISA_MIPS2_NOT_32R6_64R6;
 def TNEI : MMRel, TEQI_FT<"tnei", GPR32Opnd, II_TNEI>, TEQI_FM<0xe>,
            ISA_MIPS2_NOT_32R6_64R6;
 
 let AdditionalPredicates = [NotInMicroMips] in {
+  let isTerminator = 1 in
     def BREAK : MMRel, StdMMR6Rel, BRK_FT<"break">, BRK_FM<0xd>;
 def SYSCALL : MMRel, SYS_FT<"syscall", uimm20, II_SYSCALL>, SYS_FM<0xc>;
 }
 def TRAP : TrapBase<BREAK>;
 let AdditionalPredicates = [NotInMicroMips] in {
 def SDBBP : MMRel, SYS_FT<"sdbbp", uimm20, II_SDBBP>, SDBBP_FM, ISA_MIPS32_NOT_32R6_64R6;
 }
 
 let AdditionalPredicates = [NotInMicroMips] in {

lib/Target/NVPTX/NVPTXInstrInfo.td

NVPTXInst<(outs), (ins i32imm:$amt1, i32imm:$amt2),
             "\t.reg .b32 temp_param_reg;",
             [(callseq_start timm:$amt1, timm:$amt2)]>;
 def Callseq_End :
   NVPTXInst<(outs), (ins i32imm:$amt1, i32imm:$amt2),
             "\\} // callseq $amt1",
             [(callseq_end timm:$amt1, timm:$amt2)]>;
 
 // trap instruction
+let isTerminator = 1 in
   def trapinst : NVPTXInst<(outs), (ins), "trap;", [(trap)]>;
 
 // Call prototype wrapper
 def SDTCallPrototype : SDTypeProfile<0, 1, [SDTCisInt<0>]>;
 def CallPrototype :
   SDNode<"NVPTXISD::CallPrototype", SDTCallPrototype,
          [SDNPHasChain, SDNPOutGlue, SDNPInGlue, SDNPSideEffect]>;
 def ProtoIdent : Operand<i32> {
   let PrintMethod = "printProtoIdent";

lib/Target/SystemZ/SystemZInstrInfo.td

let isBranch = 1, isTerminator = 1 in {
   def BXLEG : BranchBinaryRSY<"bxleg", 0xEB45, GR64>;
 }
 
 //===----------------------------------------------------------------------===//
 // Trap instructions
 //===----------------------------------------------------------------------===//
 
 // Unconditional trap.
-// FIXME: This trap instruction should be marked as isTerminator, but there is
-// currently a general bug that allows non-terminators to be placed between
-// terminators. Temporarily leave this unmarked until the bug is fixed.
-let isBarrier = 1, hasCtrlDep = 1 in
+let isTerminator = 1, isBarrier = 1, hasCtrlDep = 1 in
   def Trap : Alias<4, (outs), (ins), [(trap)]>;
 
 // Conditional trap.
 let isTerminator = 1, hasCtrlDep = 1, Uses = [CC] in
   def CondTrap : Alias<4, (outs), (ins cond4:$valid, cond4:$R1), []>;
 
 // Fused compare-and-trap instructions.
 let isTerminator = 1, hasCtrlDep = 1 in {

lib/Target/X86/X86InstrSystem.td

def RDTSC : I<0x31, RawFrm, (outs), (ins), "rdtsc", [(X86rdtsc)], IIC_RDTSC>,
               TB;
 
 let Defs = [RAX, RCX, RDX] in
   def RDTSCP : I<0x01, MRM_F9, (outs), (ins), "rdtscp", [(X86rdtscp)]>, TB;
 
 // CPU flow control instructions
 
 let mayLoad = 1, mayStore = 0, hasSideEffects = 1 in {
+  let isTerminator = 1 in
     def TRAP    : I<0x0B, RawFrm, (outs), (ins), "ud2", [(trap)]>, TB;
   def UD2B    : I<0xB9, RawFrm, (outs), (ins), "ud2b", []>, TB;
 }
 
 def HLT : I<0xF4, RawFrm, (outs), (ins), "hlt", [], IIC_HLT>;
 def RSM : I<0xAA, RawFrm, (outs), (ins), "rsm", [], IIC_RSM>, TB;
 
 // Interrupt and SysCall Instructions.
 let Uses = [EFLAGS] in

lib/Target/XCore/XCoreInstrInfo.td

 let hasSideEffects=0 in
 def SETCP_1r : _F1R<0b001101, (outs), (ins GRRegs:$a), "set cp, $a", []>;
 
 let hasCtrlDep = 1 in 
 def ECALLT_1r : _F1R<0b010011, (outs), (ins GRRegs:$a),
                  "ecallt $a",
                  []>;
 
-let hasCtrlDep = 1 in 
+let hasCtrlDep = 1, isTerminator = 1 in 
 def ECALLF_1r : _F1R<0b010010, (outs), (ins GRRegs:$a),
                  "ecallf $a",
                  []>;
 
 let isCall=1, 
 // All calls clobber the link register and the non-callee-saved registers:
 Defs = [R0, R1, R2, R3, R11, LR], Uses = [SP] in {
 def BLA_1r : _F1R<0b001000, (outs), (ins GRRegs:$a),