Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[GlobalISel] Introduce G_TRAP, G_DEBUGTRAP, G_UBSANTRAP #84941

Merged
merged 7 commits into from
Mar 23, 2024

Conversation

e-kud
Copy link
Contributor

@e-kud e-kud commented Mar 12, 2024

Here we introduce three new GMIR instructions to cover a set of trap intrinsics. The idea behind it is that generic intrinsics shouldn't be used with G_INTRINSIC opcode.

These new instructions can match perfectly with existing trap ISD nodes. It allows X86, AArch64, RISCV and Mips to reuse SelectionDAG patterns for selection and avoid manual selection. However AMDGPU is an exception. It selects traps during legalization regardless SelectionDAG or GlobalISel.

Since there are not many places where traps are used, this change attempts to clean up all the usages of G_INTRINSIC with trap intrinsics. So, there is no stage when both G_TRAP and G_INTRINSIC_W_SIDE_EFFECTS(@llvm.trap) are allowed.

@llvmbot
Copy link
Collaborator

llvmbot commented Mar 12, 2024

@llvm/pr-subscribers-backend-risc-v
@llvm/pr-subscribers-llvm-ir
@llvm/pr-subscribers-backend-amdgpu
@llvm/pr-subscribers-llvm-support
@llvm/pr-subscribers-llvm-globalisel

@llvm/pr-subscribers-backend-x86

Author: Evgenii Kudriashov (e-kud)

Changes

Patch is 34.92 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/84941.diff

24 Files Affected:

  • (modified) llvm/docs/GlobalISel/GenericOpcode.rst (+30-5)
  • (modified) llvm/docs/LangRef.rst (+6)
  • (modified) llvm/include/llvm/CodeGen/GlobalISel/IRTranslator.h (+4)
  • (modified) llvm/include/llvm/CodeGen/GlobalISel/MachineIRBuilder.h (+5)
  • (modified) llvm/include/llvm/Support/TargetOpcodes.def (+5)
  • (modified) llvm/include/llvm/Target/GenericOpcodes.td (+22)
  • (modified) llvm/include/llvm/Target/GlobalISel/SelectionDAGCompat.td (+3)
  • (modified) llvm/lib/CodeGen/GlobalISel/IRTranslator.cpp (+31-16)
  • (modified) llvm/lib/Target/AArch64/AArch64InstrInfo.td (+3)
  • (modified) llvm/lib/Target/AArch64/GISel/AArch64InstructionSelector.cpp (+10-10)
  • (modified) llvm/lib/Target/AMDGPU/AMDGPULegalizerInfo.cpp (+7-5)
  • (modified) llvm/lib/Target/Mips/MipsLegalizerInfo.cpp (-5)
  • (modified) llvm/lib/Target/RISCV/GISel/RISCVInstructionSelector.cpp (-27)
  • (modified) llvm/lib/Target/X86/GISel/X86InstructionSelector.cpp (-19)
  • (modified) llvm/test/CodeGen/AArch64/GlobalISel/irtranslator-unreachable.ll (+1-1)
  • (modified) llvm/test/CodeGen/AArch64/GlobalISel/legalize-exceptions.ll (+1-1)
  • (modified) llvm/test/CodeGen/AArch64/GlobalISel/legalizer-info-validation.mir (+9)
  • (modified) llvm/test/CodeGen/AArch64/GlobalISel/select-trap.mir (+1-1)
  • (modified) llvm/test/CodeGen/AArch64/GlobalISel/uaddo-8-16-bits.mir (+26-26)
  • (modified) llvm/test/CodeGen/AMDGPU/GlobalISel/legalize-trap.mir (+2-2)
  • (renamed) llvm/test/CodeGen/Mips/GlobalISel/instruction-select/trap.mir (+6-3)
  • (modified) llvm/test/CodeGen/RISCV/GlobalISel/instruction-select/trap.mir (+2-2)
  • (modified) llvm/test/CodeGen/X86/GlobalISel/x86-select-trap.mir (+1-1)
  • (added) llvm/test/CodeGen/X86/isel-traps.ll (+73)
diff --git a/llvm/docs/GlobalISel/GenericOpcode.rst b/llvm/docs/GlobalISel/GenericOpcode.rst
index f9f9e1186460ee..eceaf2def193cc 100644
--- a/llvm/docs/GlobalISel/GenericOpcode.rst
+++ b/llvm/docs/GlobalISel/GenericOpcode.rst
@@ -779,11 +779,17 @@ G_ATOMIC_CMPXCHG
 Generic atomic cmpxchg. Expects a MachineMemOperand in addition to explicit
 operands.
 
-G_ATOMICRMW_XCHG, G_ATOMICRMW_ADD, G_ATOMICRMW_SUB, G_ATOMICRMW_AND,
-G_ATOMICRMW_NAND, G_ATOMICRMW_OR, G_ATOMICRMW_XOR, G_ATOMICRMW_MAX,
-G_ATOMICRMW_MIN, G_ATOMICRMW_UMAX, G_ATOMICRMW_UMIN, G_ATOMICRMW_FADD,
-G_ATOMICRMW_FSUB, G_ATOMICRMW_FMAX, G_ATOMICRMW_FMIN
-^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+|all_g_atomicrmw|
+^^^^^^^^^^^^^^^
+
+.. |all_g_atomicrmw| replace:: G_ATOMICRMW_XCHG, G_ATOMICRMW_ADD,
+                               G_ATOMICRMW_SUB, G_ATOMICRMW_AND,
+                               G_ATOMICRMW_NAND, G_ATOMICRMW_OR,
+                               G_ATOMICRMW_XOR, G_ATOMICRMW_MAX,
+                               G_ATOMICRMW_MIN, G_ATOMICRMW_UMAX,
+                               G_ATOMICRMW_UMIN, G_ATOMICRMW_FADD,
+                               G_ATOMICRMW_FSUB, G_ATOMICRMW_FMAX,
+                               G_ATOMICRMW_FMIN
 
 Generic atomicrmw. Expects a MachineMemOperand in addition to explicit
 operands.
@@ -922,6 +928,25 @@ The _CONVERGENT variant corresponds to an LLVM IR intrinsic marked `convergent`.
   Unlike SelectionDAG, there is no _VOID variant. Both of these are permitted
   to have zero, one, or multiple results.
 
+G_TRAP, G_DEBUGTRAP, G_UBSANTRAP
+^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+
+Represents :ref:`llvm.trap <llvm.trap>`, :ref:`llvm.debugtrap <llvm.debugtrap>`
+and :ref:`llvm.ubsantrap <llvm.ubsantrap>` that generate a target dependent
+trap instructions.
+
+.. code-block:: none
+
+  G_TRAP
+
+.. code-block:: none
+
+  G_DEBUGTRAP
+
+.. code-block:: none
+
+  G_UBSANTRAP 12
+
 Variadic Arguments
 ------------------
 
diff --git a/llvm/docs/LangRef.rst b/llvm/docs/LangRef.rst
index 77ec72f176d6ed..2650903cee5eb6 100644
--- a/llvm/docs/LangRef.rst
+++ b/llvm/docs/LangRef.rst
@@ -26869,6 +26869,8 @@ Arguments:
 
 The argument should be an MDTuple containing any number of MDStrings.
 
+.. _llvm.trap:
+
 '``llvm.trap``' Intrinsic
 ^^^^^^^^^^^^^^^^^^^^^^^^^
 
@@ -26896,6 +26898,8 @@ This intrinsic is lowered to the target dependent trap instruction. If
 the target does not have a trap instruction, this intrinsic will be
 lowered to a call of the ``abort()`` function.
 
+.. _llvm.debugtrap:
+
 '``llvm.debugtrap``' Intrinsic
 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
 
@@ -26923,6 +26927,8 @@ This intrinsic is lowered to code which is intended to cause an
 execution trap with the intention of requesting the attention of a
 debugger.
 
+.. _llvm.ubsantrap:
+
 '``llvm.ubsantrap``' Intrinsic
 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
 
diff --git a/llvm/include/llvm/CodeGen/GlobalISel/IRTranslator.h b/llvm/include/llvm/CodeGen/GlobalISel/IRTranslator.h
index bfac54a65c5b4e..5d1dcc6b747dd2 100644
--- a/llvm/include/llvm/CodeGen/GlobalISel/IRTranslator.h
+++ b/llvm/include/llvm/CodeGen/GlobalISel/IRTranslator.h
@@ -243,6 +243,10 @@ class IRTranslator : public MachineFunctionPass {
   bool translateMemFunc(const CallInst &CI, MachineIRBuilder &MIRBuilder,
                         unsigned Opcode);
 
+  /// Translate an LLVM trap intrinsic (trap, debugtrap, ubsantrap).
+  bool translateTrap(const CallInst &U, MachineIRBuilder &MIRBuilder,
+                     unsigned Opcode);
+
   void getStackGuard(Register DstReg, MachineIRBuilder &MIRBuilder);
 
   bool translateOverflowIntrinsic(const CallInst &CI, unsigned Op,
diff --git a/llvm/include/llvm/CodeGen/GlobalISel/MachineIRBuilder.h b/llvm/include/llvm/CodeGen/GlobalISel/MachineIRBuilder.h
index 4732eaf4ee27c5..90f59e7ebf27dd 100644
--- a/llvm/include/llvm/CodeGen/GlobalISel/MachineIRBuilder.h
+++ b/llvm/include/llvm/CodeGen/GlobalISel/MachineIRBuilder.h
@@ -2091,6 +2091,11 @@ class MachineIRBuilder {
                                 DstMMO, SrcMMO);
   }
 
+  /// Build and insert G_TRAP or G_DEBUGTRAP
+  MachineInstrBuilder buildTrap(bool Debug = false) {
+    return buildInstr(Debug ? TargetOpcode::G_DEBUGTRAP : TargetOpcode::G_TRAP);
+  }
+
   /// Build and insert \p Dst = G_SBFX \p Src, \p LSB, \p Width.
   MachineInstrBuilder buildSbfx(const DstOp &Dst, const SrcOp &Src,
                                 const SrcOp &LSB, const SrcOp &Width) {
diff --git a/llvm/include/llvm/Support/TargetOpcodes.def b/llvm/include/llvm/Support/TargetOpcodes.def
index 3dade14f043b60..71f0ec9159c045 100644
--- a/llvm/include/llvm/Support/TargetOpcodes.def
+++ b/llvm/include/llvm/Support/TargetOpcodes.def
@@ -834,6 +834,11 @@ HANDLE_TARGET_OPCODE(G_MEMMOVE)
 HANDLE_TARGET_OPCODE(G_MEMSET)
 HANDLE_TARGET_OPCODE(G_BZERO)
 
+/// llvm.trap, llvm.debugtrap and llvm.ubsantrap intrinsics
+HANDLE_TARGET_OPCODE(G_TRAP)
+HANDLE_TARGET_OPCODE(G_DEBUGTRAP)
+HANDLE_TARGET_OPCODE(G_UBSANTRAP)
+
 /// Vector reductions
 HANDLE_TARGET_OPCODE(G_VECREDUCE_SEQ_FADD)
 HANDLE_TARGET_OPCODE(G_VECREDUCE_SEQ_FMUL)
diff --git a/llvm/include/llvm/Target/GenericOpcodes.td b/llvm/include/llvm/Target/GenericOpcodes.td
index 8dc84fb0ba0524..05bdb7f166322e 100644
--- a/llvm/include/llvm/Target/GenericOpcodes.td
+++ b/llvm/include/llvm/Target/GenericOpcodes.td
@@ -1566,6 +1566,28 @@ def G_BZERO : GenericInstruction {
   let mayStore = true;
 }
 
+//------------------------------------------------------------------------------
+// Trap intrinsics
+//------------------------------------------------------------------------------
+def G_TRAP : GenericInstruction {
+  let OutOperandList = (outs);
+  let InOperandList = (ins);
+  let hasSideEffects = true;
+  let mayStore = true;
+}
+
+def G_DEBUGTRAP : GenericInstruction {
+  let OutOperandList = (outs);
+  let InOperandList = (ins);
+  let hasSideEffects = true;
+}
+
+def G_UBSANTRAP : GenericInstruction {
+  let OutOperandList = (outs);
+  let InOperandList = (ins untyped_imm_0:$kind);
+  let hasSideEffects = true;
+}
+
 //------------------------------------------------------------------------------
 // Bitfield extraction.
 //------------------------------------------------------------------------------
diff --git a/llvm/include/llvm/Target/GlobalISel/SelectionDAGCompat.td b/llvm/include/llvm/Target/GlobalISel/SelectionDAGCompat.td
index b1f3c500a1b6c5..4bb9929e2cf8ba 100644
--- a/llvm/include/llvm/Target/GlobalISel/SelectionDAGCompat.td
+++ b/llvm/include/llvm/Target/GlobalISel/SelectionDAGCompat.td
@@ -250,6 +250,9 @@ def : GINodeEquiv<G_ATOMICRMW_UINC_WRAP, atomic_load_uinc_wrap>;
 def : GINodeEquiv<G_ATOMICRMW_UDEC_WRAP, atomic_load_udec_wrap>;
 def : GINodeEquiv<G_FENCE, atomic_fence>;
 def : GINodeEquiv<G_PREFETCH, prefetch>;
+def : GINodeEquiv<G_TRAP, trap>;
+def : GINodeEquiv<G_DEBUGTRAP, debugtrap>;
+def : GINodeEquiv<G_UBSANTRAP, ubsantrap>;
 
 // Specifies the GlobalISel equivalents for SelectionDAG's ComplexPattern.
 // Should be used on defs that subclass GIComplexOperandMatcher<>.
diff --git a/llvm/lib/CodeGen/GlobalISel/IRTranslator.cpp b/llvm/lib/CodeGen/GlobalISel/IRTranslator.cpp
index 365870f540daeb..504ba23029b64c 100644
--- a/llvm/lib/CodeGen/GlobalISel/IRTranslator.cpp
+++ b/llvm/lib/CodeGen/GlobalISel/IRTranslator.cpp
@@ -1770,6 +1770,32 @@ bool IRTranslator::translateMemFunc(const CallInst &CI,
   return true;
 }
 
+bool IRTranslator::translateTrap(const CallInst &CI,
+                                 MachineIRBuilder &MIRBuilder,
+                                 unsigned Opcode) {
+  StringRef TrapFuncName =
+      CI.getAttributes().getFnAttr("trap-func-name").getValueAsString();
+  if (TrapFuncName.empty()) {
+    if (Opcode == TargetOpcode::G_UBSANTRAP) {
+      uint64_t Code = cast<ConstantInt>(CI.getOperand(0))->getZExtValue();
+      MIRBuilder.buildInstr(Opcode, {}, ArrayRef<llvm::SrcOp>{Code});
+    } else {
+      MIRBuilder.buildInstr(Opcode);
+    }
+    return true;
+  }
+
+  CallLowering::CallLoweringInfo Info;
+  if (Opcode == TargetOpcode::G_UBSANTRAP)
+    Info.OrigArgs.push_back({getOrCreateVRegs(*CI.getArgOperand(0)),
+                             CI.getArgOperand(0)->getType(), 0});
+
+  Info.Callee = MachineOperand::CreateES(TrapFuncName.data());
+  Info.CB = &CI;
+  Info.OrigRet = {Register(), Type::getVoidTy(CI.getContext()), 0};
+  return CLI->lowerCall(MIRBuilder, Info);
+}
+
 void IRTranslator::getStackGuard(Register DstReg,
                                  MachineIRBuilder &MIRBuilder) {
   const TargetRegisterInfo *TRI = MF->getSubtarget().getRegisterInfo();
@@ -2393,22 +2419,11 @@ bool IRTranslator::translateKnownIntrinsic(const CallInst &CI, Intrinsic::ID ID,
     return true;
   }
   case Intrinsic::trap:
+    return translateTrap(CI, MIRBuilder, TargetOpcode::G_TRAP);
   case Intrinsic::debugtrap:
-  case Intrinsic::ubsantrap: {
-    StringRef TrapFuncName =
-        CI.getAttributes().getFnAttr("trap-func-name").getValueAsString();
-    if (TrapFuncName.empty())
-      break; // Use the default handling.
-    CallLowering::CallLoweringInfo Info;
-    if (ID == Intrinsic::ubsantrap) {
-      Info.OrigArgs.push_back({getOrCreateVRegs(*CI.getArgOperand(0)),
-                               CI.getArgOperand(0)->getType(), 0});
-    }
-    Info.Callee = MachineOperand::CreateES(TrapFuncName.data());
-    Info.CB = &CI;
-    Info.OrigRet = {Register(), Type::getVoidTy(CI.getContext()), 0};
-    return CLI->lowerCall(MIRBuilder, Info);
-  }
+    return translateTrap(CI, MIRBuilder, TargetOpcode::G_DEBUGTRAP);
+  case Intrinsic::ubsantrap:
+    return translateTrap(CI, MIRBuilder, TargetOpcode::G_UBSANTRAP);
   case Intrinsic::amdgcn_cs_chain:
     return translateCallBase(CI, MIRBuilder);
   case Intrinsic::fptrunc_round: {
@@ -2949,7 +2964,7 @@ bool IRTranslator::translateUnreachable(const User &U, MachineIRBuilder &MIRBuil
     }
   }
 
-  MIRBuilder.buildIntrinsic(Intrinsic::trap, ArrayRef<Register>());
+  MIRBuilder.buildTrap();
   return true;
 }
 
diff --git a/llvm/lib/Target/AArch64/AArch64InstrInfo.td b/llvm/lib/Target/AArch64/AArch64InstrInfo.td
index 6254e68326f79d..b698d881f7e211 100644
--- a/llvm/lib/Target/AArch64/AArch64InstrInfo.td
+++ b/llvm/lib/Target/AArch64/AArch64InstrInfo.td
@@ -8423,6 +8423,9 @@ def ubsan_trap_xform : SDNodeXForm<timm, [{
   return CurDAG->getTargetConstant(N->getZExtValue() | ('U' << 8), SDLoc(N), MVT::i32);
 }]>;
 
+def gi_ubsan_trap_xform : GICustomOperandRenderer<"renderUbsanTrap">,
+  GISDNodeXFormEquiv<ubsan_trap_xform>;
+
 def ubsan_trap_imm : TImmLeaf<i32, [{
   return isUInt<8>(Imm);
 }], ubsan_trap_xform>;
diff --git a/llvm/lib/Target/AArch64/GISel/AArch64InstructionSelector.cpp b/llvm/lib/Target/AArch64/GISel/AArch64InstructionSelector.cpp
index 0f3c3cb96e6ce3..175aa9fcb8648c 100644
--- a/llvm/lib/Target/AArch64/GISel/AArch64InstructionSelector.cpp
+++ b/llvm/lib/Target/AArch64/GISel/AArch64InstructionSelector.cpp
@@ -479,6 +479,8 @@ class AArch64InstructionSelector : public InstructionSelector {
                           int OpIdx = -1) const;
   void renderLogicalImm64(MachineInstrBuilder &MIB, const MachineInstr &I,
                           int OpIdx = -1) const;
+  void renderUbsanTrap(MachineInstrBuilder &MIB, const MachineInstr &MI,
+                       int OpIdx) const;
   void renderFPImm16(MachineInstrBuilder &MIB, const MachineInstr &MI,
                      int OpIdx = -1) const;
   void renderFPImm32(MachineInstrBuilder &MIB, const MachineInstr &MI,
@@ -6140,16 +6142,6 @@ bool AArch64InstructionSelector::selectIntrinsicWithSideEffects(
     constrainSelectedInstRegOperands(*NewI, TII, TRI, RBI);
     break;
   }
-  case Intrinsic::trap:
-    MIB.buildInstr(AArch64::BRK, {}, {}).addImm(1);
-    break;
-  case Intrinsic::debugtrap:
-    MIB.buildInstr(AArch64::BRK, {}, {}).addImm(0xF000);
-    break;
-  case Intrinsic::ubsantrap:
-    MIB.buildInstr(AArch64::BRK, {}, {})
-        .addImm(I.getOperand(1).getImm() | ('U' << 8));
-    break;
   case Intrinsic::aarch64_neon_ld1x2: {
     LLT Ty = MRI.getType(I.getOperand(0).getReg());
     unsigned Opc = 0;
@@ -7644,6 +7636,14 @@ void AArch64InstructionSelector::renderLogicalImm64(
   MIB.addImm(Enc);
 }
 
+void AArch64InstructionSelector::renderUbsanTrap(MachineInstrBuilder &MIB,
+                                                 const MachineInstr &MI,
+                                                 int OpIdx) const {
+  assert(MI.getOpcode() == TargetOpcode::G_UBSANTRAP && OpIdx == 0 &&
+         "Expected G_UBSANTRAP");
+  MIB.addImm(MI.getOperand(0).getImm() | ('U' << 8));
+}
+
 void AArch64InstructionSelector::renderFPImm16(MachineInstrBuilder &MIB,
                                                const MachineInstr &MI,
                                                int OpIdx) const {
diff --git a/llvm/lib/Target/AMDGPU/AMDGPULegalizerInfo.cpp b/llvm/lib/Target/AMDGPU/AMDGPULegalizerInfo.cpp
index 0029c51231f286..dcccfd92df03fd 100644
--- a/llvm/lib/Target/AMDGPU/AMDGPULegalizerInfo.cpp
+++ b/llvm/lib/Target/AMDGPU/AMDGPULegalizerInfo.cpp
@@ -2030,6 +2030,8 @@ AMDGPULegalizerInfo::AMDGPULegalizerInfo(const GCNSubtarget &ST_,
   getActionDefinitionsBuilder({G_MEMCPY, G_MEMCPY_INLINE, G_MEMMOVE, G_MEMSET})
       .lower();
 
+  getActionDefinitionsBuilder({G_TRAP, G_DEBUGTRAP}).custom();
+
   getActionDefinitionsBuilder({G_VASTART, G_VAARG, G_BRJT, G_JUMP_TABLE,
         G_INDEXED_LOAD, G_INDEXED_SEXTLOAD,
         G_INDEXED_ZEXTLOAD, G_INDEXED_STORE})
@@ -2134,6 +2136,10 @@ bool AMDGPULegalizerInfo::legalizeCustom(
     return legalizeGetFPEnv(MI, MRI, B);
   case TargetOpcode::G_SET_FPENV:
     return legalizeSetFPEnv(MI, MRI, B);
+  case TargetOpcode::G_TRAP:
+    return legalizeTrapIntrinsic(MI, MRI, B);
+  case TargetOpcode::G_DEBUGTRAP:
+    return legalizeDebugTrapIntrinsic(MI, MRI, B);
   default:
     return false;
   }
@@ -2925,7 +2931,7 @@ bool AMDGPULegalizerInfo::legalizeGlobalValue(
       // functions that use local objects. However, if these dead functions are
       // not eliminated, we don't want a compile time error. Just emit a warning
       // and a trap, since there should be no callable path here.
-      B.buildIntrinsic(Intrinsic::trap, ArrayRef<Register>());
+      B.buildTrap();
       B.buildUndef(DstReg);
       MI.eraseFromParent();
       return true;
@@ -7270,10 +7276,6 @@ bool AMDGPULegalizerInfo::legalizeIntrinsic(LegalizerHelper &Helper,
   case Intrinsic::amdgcn_struct_buffer_atomic_fadd_v2bf16:
   case Intrinsic::amdgcn_struct_ptr_buffer_atomic_fadd_v2bf16:
     return legalizeBufferAtomic(MI, B, IntrID);
-  case Intrinsic::trap:
-    return legalizeTrapIntrinsic(MI, MRI, B);
-  case Intrinsic::debugtrap:
-    return legalizeDebugTrapIntrinsic(MI, MRI, B);
   case Intrinsic::amdgcn_rsq_clamp:
     return legalizeRsqClampIntrinsic(MI, MRI, B);
   case Intrinsic::amdgcn_ds_fadd:
diff --git a/llvm/lib/Target/Mips/MipsLegalizerInfo.cpp b/llvm/lib/Target/Mips/MipsLegalizerInfo.cpp
index f5e94235859a06..b96a3903222d73 100644
--- a/llvm/lib/Target/Mips/MipsLegalizerInfo.cpp
+++ b/llvm/lib/Target/Mips/MipsLegalizerInfo.cpp
@@ -513,11 +513,6 @@ bool MipsLegalizerInfo::legalizeIntrinsic(LegalizerHelper &Helper,
   const RegisterBankInfo &RBI = *ST.getRegBankInfo();
 
   switch (cast<GIntrinsic>(MI).getIntrinsicID()) {
-  case Intrinsic::trap: {
-    MachineInstr *Trap = MIRBuilder.buildInstr(Mips::TRAP);
-    MI.eraseFromParent();
-    return constrainSelectedInstRegOperands(*Trap, TII, TRI, RBI);
-  }
   case Intrinsic::vacopy: {
     MachinePointerInfo MPO;
     LLT PtrTy = LLT::pointer(0, 32);
diff --git a/llvm/lib/Target/RISCV/GISel/RISCVInstructionSelector.cpp b/llvm/lib/Target/RISCV/GISel/RISCVInstructionSelector.cpp
index 5738f86e7e9ff5..3103992a86c099 100644
--- a/llvm/lib/Target/RISCV/GISel/RISCVInstructionSelector.cpp
+++ b/llvm/lib/Target/RISCV/GISel/RISCVInstructionSelector.cpp
@@ -77,8 +77,6 @@ class RISCVInstructionSelector : public InstructionSelector {
                     MachineRegisterInfo &MRI) const;
   bool selectFPCompare(MachineInstr &MI, MachineIRBuilder &MIB,
                        MachineRegisterInfo &MRI) const;
-  bool selectIntrinsicWithSideEffects(MachineInstr &MI, MachineIRBuilder &MIB,
-                                      MachineRegisterInfo &MRI) const;
   void emitFence(AtomicOrdering FenceOrdering, SyncScope::ID FenceSSID,
                  MachineIRBuilder &MIB) const;
   bool selectMergeValues(MachineInstr &MI, MachineIRBuilder &MIB,
@@ -686,8 +684,6 @@ bool RISCVInstructionSelector::select(MachineInstr &MI) {
     return selectSelect(MI, MIB, MRI);
   case TargetOpcode::G_FCMP:
     return selectFPCompare(MI, MIB, MRI);
-  case TargetOpcode::G_INTRINSIC_W_SIDE_EFFECTS:
-    return selectIntrinsicWithSideEffects(MI, MIB, MRI);
   case TargetOpcode::G_FENCE: {
     AtomicOrdering FenceOrdering =
         static_cast<AtomicOrdering>(MI.getOperand(0).getImm());
@@ -1255,29 +1251,6 @@ bool RISCVInstructionSelector::selectFPCompare(MachineInstr &MI,
   return true;
 }
 
-bool RISCVInstructionSelector::selectIntrinsicWithSideEffects(
-    MachineInstr &MI, MachineIRBuilder &MIB, MachineRegisterInfo &MRI) const {
-  assert(MI.getOpcode() == TargetOpcode::G_INTRINSIC_W_SIDE_EFFECTS &&
-         "Unexpected opcode");
-  // Find the intrinsic ID.
-  unsigned IntrinID = cast<GIntrinsic>(MI).getIntrinsicID();
-
-  // Select the instruction.
-  switch (IntrinID) {
-  default:
-    return false;
-  case Intrinsic::trap:
-    MIB.buildInstr(RISCV::UNIMP, {}, {});
-    break;
-  case Intrinsic::debugtrap:
-    MIB.buildInstr(RISCV::EBREAK, {}, {});
-    break;
-  }
-
-  MI.eraseFromParent();
-  return true;
-}
-
 void RISCVInstructionSelector::emitFence(AtomicOrdering FenceOrdering,
                                          SyncScope::ID FenceSSID,
                                          MachineIRBuilder &MIB) const {
diff --git a/llvm/lib/Target/X86/GISel/X86InstructionSelector.cpp b/llvm/lib/Target/X86/GISel/X86InstructionSelector.cpp
index 8e0f61a855661b..9be3812300af12 100644
--- a/llvm/lib/Target/X86/GISel/X86InstructionSelector.cpp
+++ b/llvm/lib/Target/X86/GISel/X86InstructionSelector.cpp
@@ -119,8 +119,6 @@ class X86InstructionSelector : public InstructionSelector {
                        MachineFunction &MF) const;
   bool selectSelect(MachineInstr &I, MachineRegisterInfo &MRI,
                     MachineFunction &MF) const;
-  bool selectIntrinsicWSideEffects(MachineInstr &I, MachineRegisterInfo &MRI,
-                                   MachineFunction &MF) const;
 
   // emit insert subreg instruction and insert it before MachineInstr &I
   bool emitInsertSubreg(unsigned DstReg, unsigned SrcReg, MachineInstr &I,
@@ -434,8 +432,6 @@ bool X86InstructionSelector::select(MachineInstr &I) {
     return selectMulDivRem(I, MRI, MF);
   case TargetOpcode::G_SELECT:
     return selectSelect(I, MRI, MF);
-  case TargetOpcode::G_INTRINSIC_W_SIDE_EFFECTS:
-    return selectIntrinsicWSideEffects(I, MRI, MF);
   }
 
   return false;
@@ -1834,21 +1830,6 @@ bool X86InstructionSelector::selectSelect(MachineInstr &I,
   return true;
 }
 
-bool X86InstructionSelector::selectIntrinsicWSideEffects(
-    MachineInstr &I, MachineRegisterInfo &MRI, MachineFunction &MF) const {
-
-  assert(I.getOpcode() == TargetOpcode::G_INTRINSIC_W_SIDE_EFFECTS &&
-         "unexpected instruction");
-
-  if (I.getOperand(0).getIntrinsicID() != Intrinsic::trap)
-    return false;
-
-  BuildMI(*I.getParent(), I, I.getDebugLoc(), TII.get(X86::TRAP));
-
-  I.eraseFromParent();
-  return true;
-}
-
 InstructionSelector *
 llvm::createX86InstructionSelector(const X86TargetMachine &TM,
                                    X86Subtarget &Subtarget,
diff --git a/llvm/test/CodeGen/AArch64/GlobalISel/irtranslator-unreachable.ll b/llvm/test/CodeGen/AArch64/GlobalISel/irtranslator-unreachable.ll
index fe9427d2678a16..edae903fae848f 100644
--- a/llvm/test/CodeGen/AArch64/GlobalISel/irtranslator-unreachable.ll
+++ b/llvm/test/CodeGen/AArch64/GlobalISel/irtranslator-unreachable.ll
@@ -6,7 +6,7 @@ declare void @llvm.trap()
 define void @unreachable() {
   ; CHECK-LABEL: name: unreachable
   ; CHECK: bb.1 (%ir-block.0):
-  ; CHECK-NEXT:   G_INTRINSIC_W_SIDE_EFFECTS intrinsic(@llvm.trap)
+  ; CHECK-NEXT:   G_TRAP
   unr...
[truncated]

@e-kud
Copy link
Contributor Author

e-kud commented Mar 12, 2024

The idea of having these opcodes has been suggested by @arsenm in #83498 (comment)

I find it useful as X86, AArch64, RISCV and Mips can avoid manual selection. AMDGPU is an exception as it lowers traps during legalization even using Selection DAG.

I'm not sure whether it is a good idea or not to edit all targets at once. But I'd like to avoid intermediate stage when both G_*TRAP and G_INTRINSIC_W_SIDE_EFFECTS intrinsic(@llvm.*trap) are allowed.

I've tried to cleanup all G_INTRINSIC_W_SIDE_EFFECTS.*trap usages as well as llvm.*trap.

Let me know if the change is undesired, then I'll simply merge aforementioned fix for X86.

Probably extra people are required to review Mips and AMDGPU changes.

Copy link
Contributor

@arsenm arsenm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm except unrelated doc change

llvm/docs/GlobalISel/GenericOpcode.rst Outdated Show resolved Hide resolved
@topperc
Copy link
Collaborator

topperc commented Mar 12, 2024

The idea of having these opcodes has been suggested by @arsenm in #83498 (comment)

I find it useful as X86, AArch64, RISCV and Mips can avoid manual selection. AMDGPU is an exception as it lowers traps during legalization even using Selection DAG.

I'm not sure whether it is a good idea or not to edit all targets at once. But I'd like to avoid intermediate stage when both G_*TRAP and G_INTRINSIC_W_SIDE_EFFECTS intrinsic(@llvm.*trap) are allowed.

I've tried to cleanup all G_INTRINSIC_W_SIDE_EFFECTS.*trap usages as well as llvm.*trap.

Can you put this in the PR description so it will end up in the commit message.

@e-kud
Copy link
Contributor Author

e-kud commented Mar 14, 2024

Can you put this in the PR description so it will end up in the commit message.

@topperc I've rephrased in more general terms.

I've noticed that there are checks for GMIR opcodes in MachineVerifier. Should I add sanity checks that G_TRAP and G_DEBUGTRAP have no args and G_UBSANTRAP has only an i8 immediate argument?

@arsenm
Copy link
Contributor

arsenm commented Mar 15, 2024

Can you put this in the PR description so it will end up in the commit message.

@topperc I've rephrased in more general terms.

I've noticed that there are checks for GMIR opcodes in MachineVerifier. Should I add sanity checks that G_TRAP and G_DEBUGTRAP have no args and G_UBSANTRAP has only an i8 immediate argument?

You should add the tests. I believe the operand number checks should work by default, but you might need code to verify the immediate operand

@e-kud e-kud merged commit d365a45 into llvm:main Mar 23, 2024
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants