AMDGPU/GlobalISel: Support llvm.trap and llvm.debugtrap intrinsics

[hsmhsm]

Summary

Lower trap and debugtrap intrinsics to AMDGPU machine instruction(s).

[arsenm]

llvm/lib/Target/AMDGPU/AMDGPULegalizerInfo.cpp

3571–3572

This is directly copying the physical register into another physical register. We don't want that. This needs to go through an intermediate virtual register copy

llvm/test/CodeGen/AMDGPU/GlobalISel/llvm.debugtrap.ll

3–14 ↗

(On Diff #244864)

Shouldn't run the DAG tests in the GlobalIsel test directory

llvm/test/CodeGen/AMDGPU/GlobalISel/llvm.trap.ll

96

Can you add another test that uses the stack?

Ideally we would also have a test in a non-kernel function, but I know that won't work right now since we don't handle the special argument inputs yet

[arsenm]

llvm/lib/Target/AMDGPU/AMDGPULegalizerInfo.cpp

3571–3572

Can you add another assert in loadInputValue? It already asserts assert(Arg->getRegister().isPhysical()), but should also assert the result is virtual

[arsenm]

Can you also add a MIR test to make sure the intermediate virtual copy is added? You'll need to explicitly add the queueptr in the MIR MachineFunctionInfo

llvm/test/CodeGen/AMDGPU/GlobalISel/llvm.trap.ll

96

Actually, never mind. A non-kernel function should work. Calling it will not

[hsmhsm]

Take care of review comments by arsenm.

1. Introduce intermediate virtual register copy
2. Remove DAG tests in the GlobalIsel test directory
3. Add LLVM IRs that uses the stack
4. Assert that destination register is virtual within loadInputValue()

[hsmhsm]

Regarding the sharing of original ISelDAG test within GlobalISel, it seems to be not working since there are checks like below in case of ISelDAG path. Hence, let's keep the both the tests separate for now.

; MESA-TRAP: .section .AMDGPU.config
; MESA-TRAP: .long 47180
; MESA-TRAP-NEXT: .long 208

[arsenm]

In D74688#1879573, @hsmhsm wrote:

Regarding the sharing of original ISelDAG test within GlobalISel, it seems to be not working since there are checks like below in case of ISelDAG path. Hence, let's keep the both the tests separate for now.

; MESA-TRAP: .section .AMDGPU.config
; MESA-TRAP: .long 47180
; MESA-TRAP-NEXT: .long 208

I would expect these to be the same in both?

llvm/lib/Target/AMDGPU/AMDGPULegalizerInfo.cpp

3573

loadInputValue calls getLiveInRegister, you shouldn't need it here

3574

Why do you need to set the class? It shouldn't be necessary at this point

llvm/test/CodeGen/AMDGPU/GlobalISel/llvm.trap.ll

96

Should still add a function test. Also one that has a separate, explicit use of the queue.ptr intrinsic wouldn't hurt either

[hsmhsm]

Share the original DAG test for GlobalISel, and do not re-invent new one here.

[hsmhsm]

llvm/lib/Target/AMDGPU/AMDGPULegalizerInfo.cpp

3573

My understanding of your earlier review comments is:

1. loadInputValue() should expect, source register as physical register, and destination register as virtual register, and we should have assertions for these accordingly. The assertion for later was missing, that I added as per your review suggestion.
2. Now, going by above, it is the responsibility of the caller of loadInputValue() to make sure that it passes destination register as virtual register.
3. Hence, I created a destination virtual register here by calling getLiveInRegister(). The another call within loadInputValue() is for source physical register.

I am not getting what am I missing here.

3574

We need to set a register class for destination virtual register. otherwise, we get an assertion failure during compilation saying that the register type is not valid.

If it is not suppose to be setting here, where else should I be setting it?
OR
do I need to handle it in a different way? Please suggest it in more detail.

llvm/test/CodeGen/AMDGPU/GlobalISel/llvm.trap.ll

96

Actually, I am bit confused about writing test cases here. Let's start from scratch here.

1. As you suggested, I have shared the same original DAG test for GlobalISel, without re-inventing the same for GlobalISel. I was making some mistakes earlier, that I learned and corrected it, and sharing of original DAG test works fine now.
2. Now, starting from this point, please suggest how should I further continue here.
   • What are the additional tests that need to be covered here, and why?
   • Do I need to add the additional tests within original DAG test, and make sure both ISelDAG path and GlobalSel path work fine for these additional tests?
   • Or these are specific to only GlobalISel path for some specific reason?

[arsenm]

llvm/lib/Target/AMDGPU/AMDGPULegalizerInfo.cpp

3573

loadInputValue loads a physical register copy into a virtual register. This was written for the use by the intrinsics, which have a virtual register result write to already. In this case you don't have or really need one.

What you really want is a version of getLiveInRegister that ensures the copy is inserted, as the second half of loadInputValue does. What you have now I think happens to work, but confusingly since the loadInputValue call will insert it

You may want to split this part into a helper function:

// Insert the argument copy if it doens't already exist.
// FIXME: It seems EmitLiveInCopies isn't called anywhere?
if (!MRI.getVRegDef(LiveIn)) {
  // FIXME: Should have scoped insert pt
  MachineBasicBlock &OrigInsBB = B.getMBB();
  auto OrigInsPt = B.getInsertPt();

  MachineBasicBlock &EntryMBB = B.getMF().front();
  EntryMBB.addLiveIn(Arg->getRegister());
  B.setInsertPt(EntryMBB, EntryMBB.begin());
  B.buildCopy(LiveIn, Arg->getRegister());

  B.setInsertPt(OrigInsBB, OrigInsPt);
}

3574

The register class is only tangentially related to the type. I think you somehow ended up creating a virtual register without setting the type on it. Did something already add the queue ptr as a live in before this lowering?

I also would not call this VDstReg, as that makes it sound like a VGPR.

[arsenm]

Another possible strategy would be to emit a call to the queue.ptr intrinsic, and allow that to be legalized

[hsmhsm]

Take care of further review comments by arsenm.

1. Add an another version of getLiveInRegister() which works with TargetRegisterClass
2. While copying to destination virtual register, make sure that it is defined
3. Bit of code refactoring around related part of the source.

[hsmhsm]

llvm/lib/Target/AMDGPU/AMDGPULegalizerInfo.cpp

3573

I have taken care of it. Now, before making copy to destination virtual register, we ensure that it is defined.

3574

I have taken care of it. I just needed an another version of getLiveInRegister() which works with TargetRegisterClass.

[arsenm]

llvm/lib/Target/AMDGPU/AMDGPULegalizerInfo.cpp

2238–2239

I still don't think you should need the register class set. This should still have the type set on the virtual register

2244–2247

I think it would be better to have this only take the physical register input, and return the livein virtreg. This wouldn't have the
// Destination virtual register is already defined, just insert copy

case. The intrinsic lowering cases would then be responsible for inserting the extra copy to the expected result

2315

This shouldn't be called twice

2325–2326

Move the assignment up to avoid the weird !()

3569

Ditto

3576

Remove DstReg and just directly refer to the physical register

llvm/lib/Target/AMDGPU/AMDGPULegalizerInfo.h

96

Should use MCRegister for Reg to make it clear it's physical.

[hsmhsm]

Changes as per further review comments:

1. First, I would like to make sure that all the *necessary* foundational issues are handled before actually discussing the crux of some assertion failure, that we hit, if we do not assign a register class to a virtual live-in register of SGPR01.
2. So, I have taken care of all your previous review comments here, and I have deliberately put assertions about physical registers where it is expected.
3. Let's ensure these changes are as per your satisfaction, before moving to next point of discussion. Please let me know if I need to take care any remaining issues here before actually discussing assertion failure related issues.

[hsmhsm]

llvm/lib/Target/AMDGPU/AMDGPULegalizerInfo.cpp

2238–2239

I have taken care of it. But, we hit with an assertion, if we do not set register class. However, let's assume for now that this is what expected here, and later discuss this assertion issue, once we are fine with all the foundational changes.

2244–2247

taken care

2315

taken care

2325–2326

taken care

3569

taken care

llvm/lib/Target/AMDGPU/AMDGPULegalizerInfo.h

96

BuildCopy seems not work with MCRegister, hence added an assertion to ensure that *PhyReg* is indeed physical register.

[arsenm]

llvm/lib/Target/AMDGPU/AMDGPULegalizerInfo.cpp

2244–2247

This is halfway there. You're still passing in both the live in physical register and the corresponding virtual register. I was thinking you would call getLiveInRegister with just the physical register, and it would be responsible for finding out the virtual register, and inserting the entry block copy by calling insertLiveInCopy. loadInputValue then wouldn't need to worry about ensuring the entry block copy was inserted like it does now.

3574

Physical registers don't have types. This will just assert. Here you know the type is just LLT::pointer(CONSTANT_ADDRESSS, 64)

[hsmhsm]

Take care of further review comments by arsenm.

[hsmhsm]

llvm/lib/Target/AMDGPU/AMDGPULegalizerInfo.cpp

2244–2247

Taken care

[t-tye]

I believe that the debug_trap trap handler support no longer needs the queue_ptr to be passed in as it is internally computing it from the dootbell ID available from a GETREG together with a doorbell to queue mapping maintained by the ROCm Runtim that is accessible through the TMB register.

So should that change be reflect in this to simplify things? This is does change the ABI so needs the HSA ABI number to be incremented in the ELF header and AMDGOUUsage documentation updated. However, from the compilers point of view it is not ABI breaking as old code will still work, as it is generating code to set the queue_ptr that is unnecessary.

Adding @kzhuravl for ELF ABI version help.

[hsmhsm]

In D74688#1888262, @t-tye wrote:

I believe that the debug_trap trap handler support no longer needs the queue_ptr to be passed in as it is internally computing it from the dootbell ID available from a GETREG together with a doorbell to queue mapping maintained by the ROCm Runtim that is accessible through the TMB register.

So should that change be reflect in this to simplify things? This is does change the ABI so needs the HSA ABI number to be incremented in the ELF header and AMDGOUUsage documentation updated. However, from the compilers point of view it is not ABI breaking as old code will still work, as it is generating code to set the queue_ptr that is unnecessary.

Adding @kzhuravl for ELF ABI version help.

Hi Tony,

If you are specifically asking for llvm.debugtrap() intrinsic here, then, we are already taken care of it, and we are not adding queue_ptr in this case, as you can see from the code changes at AMDGPULegalizerInfo.cpp:3602. The queue_ptr related discussions here are only specific to llvm.trap() intrinsic.

[arsenm]

LGTM

[hsmhsm]

Couple of additional required changes

[1] SGPR01 is a new register created within legalizeTrapIntrinsic(), and we actually need to get its type as LLT::scalar(64).
[2] Call to insertLiveInCopy() within getLiveInRegister() is only required if the copy is required from physical to virtual register.
[3] When call to insertLiveInCopy() within getLiveInRegister() is required, make sure that it is called irrespective of whether the virtual register is newly created within getLiveInRegister() or not.

[t-tye]

In D74688#1888777, @hsmhsm wrote:

In D74688#1888262, @t-tye wrote:

I believe that the debug_trap trap handler support no longer needs the queue_ptr to be passed in as it is internally computing it from the dootbell ID available from a GETREG together with a doorbell to queue mapping maintained by the ROCm Runtim that is accessible through the TMB register.

So should that change be reflect in this to simplify things? This is does change the ABI so needs the HSA ABI number to be incremented in the ELF header and AMDGOUUsage documentation updated. However, from the compilers point of view it is not ABI breaking as old code will still work, as it is generating code to set the queue_ptr that is unnecessary.

Adding @kzhuravl for ELF ABI version help.

Hi Tony,

If you are specifically asking for llvm.debugtrap() intrinsic here, then, we are already taken care of it, and we are not adding queue_ptr in this case, as you can see from the code changes at AMDGPULegalizerInfo.cpp:3602. The queue_ptr related discussions here are only specific to llvm.trap() intrinsic.

Looking at AMDGPULegalizerInfo.cpp:3602 it appears the queue_ptr is still being set up so not sure what you mean that it is already being taken care of. What I describe is true for both llvm.trap() and llvm.debug_trap(). Should the setup of the queu_ptr be removed since it is no longer needed to support either llvm.trap() or llvm.debug_trap()? Doing so would need a change in AMDGPUUsage, a change to the ELF HSA ABI number, and corresponding ROCm loader changes. There may also need to be a plan to support older ROCm releases.

[hsmhsm]

In D74688#1900248, @t-tye wrote:

In D74688#1888777, @hsmhsm wrote:

In D74688#1888262, @t-tye wrote:

I believe that the debug_trap trap handler support no longer needs the queue_ptr to be passed in as it is internally computing it from the dootbell ID available from a GETREG together with a doorbell to queue mapping maintained by the ROCm Runtim that is accessible through the TMB register.

So should that change be reflect in this to simplify things? This is does change the ABI so needs the HSA ABI number to be incremented in the ELF header and AMDGOUUsage documentation updated. However, from the compilers point of view it is not ABI breaking as old code will still work, as it is generating code to set the queue_ptr that is unnecessary.

Adding @kzhuravl for ELF ABI version help.

Hi Tony,

If you are specifically asking for llvm.debugtrap() intrinsic here, then, we are already taken care of it, and we are not adding queue_ptr in this case, as you can see from the code changes at AMDGPULegalizerInfo.cpp:3602. The queue_ptr related discussions here are only specific to llvm.trap() intrinsic.

Looking at AMDGPULegalizerInfo.cpp:3602 it appears the queue_ptr is still being set up so not sure what you mean that it is already being taken care of.

The line number has changed with latest changes. I was basically referring to the function - AMDGPULegalizerInfo::legalizeDebugTrapIntrinsic() by assuming that you referring here only debugtrap() inntrinsic.

What I describe is true for both llvm.trap() and llvm.debug_trap(). Should the setup of the queu_ptr be removed since it is no longer needed to support either llvm.trap() or llvm.debug_trap()? Doing so would need a change in AMDGPUUsage, a change to the ELF HSA ABI number, and corresponding ROCm loader changes. There may also need to be a plan to support older ROCm releases.

I suggest that let's check-in this change so that trap and debug trap support looks similar both in ISELDAG and GLOBALISEL PATH (by setting-up queue_ptr) for now. And, we should take-up what you mention above (to remove queue_ptr) as a separate activity, first in ISELDAG path, and then in GLOBALISEL path.

[t-tye]

In D74688#1900255, @hsmhsm wrote:

In D74688#1900248, @t-tye wrote:

In D74688#1888777, @hsmhsm wrote:

In D74688#1888262, @t-tye wrote:

I believe that the debug_trap trap handler support no longer needs the queue_ptr to be passed in as it is internally computing it from the dootbell ID available from a GETREG together with a doorbell to queue mapping maintained by the ROCm Runtim that is accessible through the TMB register.

So should that change be reflect in this to simplify things? This is does change the ABI so needs the HSA ABI number to be incremented in the ELF header and AMDGOUUsage documentation updated. However, from the compilers point of view it is not ABI breaking as old code will still work, as it is generating code to set the queue_ptr that is unnecessary.

Adding @kzhuravl for ELF ABI version help.

Hi Tony,

If you are specifically asking for llvm.debugtrap() intrinsic here, then, we are already taken care of it, and we are not adding queue_ptr in this case, as you can see from the code changes at AMDGPULegalizerInfo.cpp:3602. The queue_ptr related discussions here are only specific to llvm.trap() intrinsic.

Looking at AMDGPULegalizerInfo.cpp:3602 it appears the queue_ptr is still being set up so not sure what you mean that it is already being taken care of.

The line number has changed with latest changes. I was basically referring to the function - AMDGPULegalizerInfo::legalizeDebugTrapIntrinsic() by assuming that you referring here only debugtrap() inntrinsic.

OK I see that now. Thanks.

What I describe is true for both llvm.trap() and llvm.debug_trap(). Should the setup of the queu_ptr be removed since it is no longer needed to support either llvm.trap() or llvm.debug_trap()? Doing so would need a change in AMDGPUUsage, a change to the ELF HSA ABI number, and corresponding ROCm loader changes. There may also need to be a plan to support older ROCm releases.

I suggest that let's check-in this change so that trap and debug trap support looks similar both in ISELDAG and GLOBALISEL PATH (by setting-up queue_ptr) for now. And, we should take-up what you mention above (to remove queue_ptr) as a separate activity, first in ISELDAG path, and then in GLOBALISEL path.

OK that sounds reasonable. Thanks.

[arsenm]

llvm/lib/Target/AMDGPU/AMDGPULegalizerInfo.cpp

3573

I would just use the literal register value below rather than have a variable for it. At least make this a const Register

3574

The type here should really be LLT::pointer(CONSTANT_ADDRESS, 64)

[hsmhsm]

Take care of latest review comments by matt about type setting of virtual register.

llvm/lib/Target/AMDGPU/AMDGPULegalizerInfo.h

public:
   bool legalizeFFloor(MachineInstr &MI, MachineRegisterInfo &MRI,
                       MachineIRBuilder &B) const;
 
   bool legalizeBuildVector(MachineInstr &MI, MachineRegisterInfo &MRI,
                            MachineIRBuilder &B) const;
 
   Register getLiveInRegister(MachineRegisterInfo &MRI,
                              Register Reg, LLT Ty) const;
+  Register getLiveInRegister(MachineRegisterInfo &MRI, Register Reg,
+                             const TargetRegisterClass *RC) const;
+  void insertLiveInCopy(MachineIRBuilder &B, MachineRegisterInfo &MRI,
+                        Register LiveIn, Register Reg) const;
+
+  const ArgDescriptor *
+  getArgDescriptor(MachineIRBuilder &B,
+                   AMDGPUFunctionArgInfo::PreloadedValue ArgType) const;
 
   bool loadInputValue(Register DstReg, MachineIRBuilder &B,
                       const ArgDescriptor *Arg) const;
   bool legalizePreloadedArgIntrin(
     MachineInstr &MI, MachineRegisterInfo &MRI, MachineIRBuilder &B,
     AMDGPUFunctionArgInfo::PreloadedValue ArgType) const;
 
   bool legalizeUDIV_UREM(MachineInstr &MI, MachineRegisterInfo &MRI,
public:
 
   bool legalizeSBufferLoad(
     MachineInstr &MI, MachineIRBuilder &B,
     GISelChangeObserver &Observer) const;
 
   bool legalizeAtomicIncDec(MachineInstr &MI,  MachineIRBuilder &B,
                             bool IsInc) const;
 
+  bool legalizeTrapIntrinsic(MachineInstr &MI, MachineRegisterInfo &MRI,
+                             MachineIRBuilder &B) const;
+  bool legalizeDebugTrapIntrinsic(MachineInstr &MI, MachineRegisterInfo &MRI,
+                                  MachineIRBuilder &B) const;
+
   bool legalizeIntrinsic(MachineInstr &MI, MachineIRBuilder &B,
                          GISelChangeObserver &Observer) const override;
 };
 } // End llvm namespace.
 #endif

llvm/lib/Target/AMDGPU/AMDGPULegalizerInfo.cpp

 }
 
 Register AMDGPULegalizerInfo::getLiveInRegister(MachineRegisterInfo &MRI,
                                                 Register Reg, LLT Ty) const {
   Register LiveIn = MRI.getLiveInVirtReg(Reg);
   if (LiveIn)
     return LiveIn;
 
-  Register NewReg = MRI.createGenericVirtualRegister(Ty);
-  MRI.addLiveIn(Reg, NewReg);
-  return NewReg;
+  LiveIn = MRI.createGenericVirtualRegister(Ty);
+  MRI.addLiveIn(Reg, LiveIn);
+
+  return LiveIn;
+}
+
+Register
+AMDGPULegalizerInfo::getLiveInRegister(MachineRegisterInfo &MRI, Register Reg,
+                                       const TargetRegisterClass *RC) const {
+  Register LiveIn = MRI.getLiveInVirtReg(Reg);
+  if (LiveIn)
+    return LiveIn;
+
+  LiveIn = MRI.createVirtualRegister(RC);
+  MRI.addLiveIn(Reg, LiveIn);
+
+  return LiveIn;
+}
+
+void AMDGPULegalizerInfo::insertLiveInCopy(MachineIRBuilder &B,
+                                           MachineRegisterInfo &MRI,
+                                           Register LiveIn,
+                                           Register Reg) const {
+  // Insert the live-in copy, if required, by defining destination virtual
+  // register.
+  // FIXME: It seems EmitLiveInCopies isn't called anywhere?
+  if (!MRI.getVRegDef(LiveIn)) {
+    // FIXME: Should have scoped insert pt
+    MachineBasicBlock &OrigInsBB = B.getMBB();
+    auto OrigInsPt = B.getInsertPt();
+
+    MachineBasicBlock &EntryMBB = B.getMF().front();
+    EntryMBB.addLiveIn(Reg);
+    B.setInsertPt(EntryMBB, EntryMBB.begin());
+    B.buildCopy(LiveIn, Reg);
+
+    B.setInsertPt(OrigInsBB, OrigInsPt);
+  } else {
+    // Destination virtual register is already defined, just insert copy.
+    B.buildCopy(LiveIn, Reg);
+  }
+}
+
+const ArgDescriptor *AMDGPULegalizerInfo::getArgDescriptor(
+    MachineIRBuilder &B, AMDGPUFunctionArgInfo::PreloadedValue ArgType) const {
+  const SIMachineFunctionInfo *MFI = B.getMF().getInfo<SIMachineFunctionInfo>();
+  const ArgDescriptor *Arg;
+  const TargetRegisterClass *RC;
+  std::tie(Arg, RC) = MFI->getPreloadedValue(ArgType);
+  if (!Arg) {
+    LLVM_DEBUG(dbgs() << "Required arg register missing\n");
+    return nullptr;
+  }
+  return Arg;
 }
 
 bool AMDGPULegalizerInfo::loadInputValue(Register DstReg, MachineIRBuilder &B,
                                          const ArgDescriptor *Arg) const {
   if (!Arg->isRegister() || !Arg->getRegister().isValid())
     return false; // TODO: Handle these
 
-  assert(Arg->getRegister().isPhysical());
+  Register SrcReg = Arg->getRegister();
+  assert(SrcReg.isPhysical());
+  assert(DstReg.isVirtual());
 
   MachineRegisterInfo &MRI = *B.getMRI();
 
   LLT Ty = MRI.getType(DstReg);
-  Register LiveIn = getLiveInRegister(MRI, Arg->getRegister(), Ty);
+  Register LiveIn = getLiveInRegister(MRI, SrcReg, Ty);
 
   if (Arg->isMasked()) {
     // TODO: Should we try to emit this once in the entry block?
     const LLT S32 = LLT::scalar(32);
     const unsigned Mask = Arg->getMask();
     const unsigned Shift = countTrailingZeros<unsigned>(Mask);
 
     Register AndMaskSrc = LiveIn;
 
     if (Shift != 0) {
       auto ShiftAmt = B.buildConstant(S32, Shift);
       AndMaskSrc = B.buildLShr(S32, LiveIn, ShiftAmt).getReg(0);
     }
 
     B.buildAnd(DstReg, AndMaskSrc, B.buildConstant(S32, Mask >> Shift));
-  } else
-    B.buildCopy(DstReg, LiveIn);
+  } else {
+    // Insert the argument virtual register copy
+    insertLiveInCopy(B, MRI, DstReg, LiveIn);
+  }
 
   // Insert the argument copy if it doens't already exist.
-  // FIXME: It seems EmitLiveInCopies isn't called anywhere?
-  if (!MRI.getVRegDef(LiveIn)) {
-    // FIXME: Should have scoped insert pt
-    MachineBasicBlock &OrigInsBB = B.getMBB();
-    auto OrigInsPt = B.getInsertPt();
-
-    MachineBasicBlock &EntryMBB = B.getMF().front();
-    EntryMBB.addLiveIn(Arg->getRegister());
-    B.setInsertPt(EntryMBB, EntryMBB.begin());
-    B.buildCopy(LiveIn, Arg->getRegister());
-
-    B.setInsertPt(OrigInsBB, OrigInsPt);
-  }
+  insertLiveInCopy(B, MRI, LiveIn, SrcReg);
 
   return true;
 }
 
 bool AMDGPULegalizerInfo::legalizePreloadedArgIntrin(
-  MachineInstr &MI,
-  MachineRegisterInfo &MRI,
-  MachineIRBuilder &B,
+    MachineInstr &MI, MachineRegisterInfo &MRI, MachineIRBuilder &B,
     AMDGPUFunctionArgInfo::PreloadedValue ArgType) const {
   B.setInstr(MI);
 
-  const SIMachineFunctionInfo *MFI = B.getMF().getInfo<SIMachineFunctionInfo>();
-
   const ArgDescriptor *Arg;
-  const TargetRegisterClass *RC;
-  std::tie(Arg, RC) = MFI->getPreloadedValue(ArgType);
-  if (!Arg) {
-    LLVM_DEBUG(dbgs() << "Required arg register missing\n");
+  if (!(Arg = getArgDescriptor(B, ArgType)))
+    return false;
+
+  if (!loadInputValue(MI.getOperand(0).getReg(), B, Arg))
     return false;
-  }
 
-  if (loadInputValue(MI.getOperand(0).getReg(), B, Arg)) {
-    MI.eraseFromParent();
-    return true;
-  }
 
-  return false;
-}
-
 bool AMDGPULegalizerInfo::legalizeFDIV(MachineInstr &MI,
                                        MachineRegisterInfo &MRI,
                                        MachineIRBuilder &B) const {
   B.setInstr(MI);
   Register Dst = MI.getOperand(0).getReg();
   LLT DstTy = MRI.getType(Dst);
   LLT S16 = LLT::scalar(16);
   LLT S32 = LLT::scalar(32);
if (!isPowerOf2_32(Size)) {
     else
       Helper.widenScalarDst(MI, getPow2ScalarType(Ty), 0);
   }
 
   Observer.changedInstr(MI);
   return true;
 }
 
+bool AMDGPULegalizerInfo::legalizeTrapIntrinsic(MachineInstr &MI,
+                                                MachineRegisterInfo &MRI,
+                                                MachineIRBuilder &B) const {
+  B.setInstr(MI);
+
+  // Is non-HSA path or trap-handler disabled? then, insert s_endpgm instruction
+  if (ST.getTrapHandlerAbi() != GCNSubtarget::TrapHandlerAbiHsa ||
+      !ST.isTrapHandlerEnabled()) {
+    B.buildInstr(AMDGPU::S_ENDPGM).addImm(0);
+  } else {
+    // Pass queue pointer to trap handler as input, and insert trap instruction
+    // Reference: https://llvm.org/docs/AMDGPUUsage.html#trap-handler-abi
+    const ArgDescriptor *Arg;
+    if (!(Arg = getArgDescriptor(B, AMDGPUFunctionArgInfo::QUEUE_PTR)))
+      return false;
+    MachineRegisterInfo &MRI = *B.getMRI();
+    Register DstReg(AMDGPU::SGPR0_SGPR1);
+    Register LiveIn = getLiveInRegister(MRI, DstReg, &AMDGPU::SGPR_64RegClass);
+    if (!loadInputValue(LiveIn, B, Arg))
+      return false;
+    B.buildCopy(DstReg, LiveIn);
+    B.buildInstr(AMDGPU::S_TRAP)
+        .addImm(GCNSubtarget::TrapIDLLVMTrap)
+        .addReg(DstReg, RegState::Implicit);
+  }
+
+  MI.eraseFromParent();
+  return true;
+}
+
+bool AMDGPULegalizerInfo::legalizeDebugTrapIntrinsic(
+    MachineInstr &MI, MachineRegisterInfo &MRI, MachineIRBuilder &B) const {
+  B.setInstr(MI);
+
+  // Is non-HSA path or trap-handler disabled? then, report a warning
+  // accordingly
+  if (ST.getTrapHandlerAbi() != GCNSubtarget::TrapHandlerAbiHsa ||
+      !ST.isTrapHandlerEnabled()) {
+    DiagnosticInfoUnsupported NoTrap(B.getMF().getFunction(),
+                                     "debugtrap handler not supported",
+                                     MI.getDebugLoc(), DS_Warning);
+    LLVMContext &Ctx = B.getMF().getFunction().getContext();
+    Ctx.diagnose(NoTrap);
+  } else {
+    // Insert debug-trap instruction
+    B.buildInstr(AMDGPU::S_TRAP).addImm(GCNSubtarget::TrapIDLLVMDebugTrap);
+  }
+
+  MI.eraseFromParent();
+  return true;
+}
+
 bool AMDGPULegalizerInfo::legalizeIntrinsic(MachineInstr &MI,
                                             MachineIRBuilder &B,
                                             GISelChangeObserver &Observer) const {
   MachineRegisterInfo &MRI = *B.getMRI();
 
   // Replace the use G_BRCOND with the exec manipulate and branch pseudos.
   auto IntrID = MI.getIntrinsicID();
   switch (IntrID) {
bool AMDGPULegalizerInfo::legalizeIntrinsic(MachineInstr &MI,
   case Intrinsic::amdgcn_struct_buffer_atomic_dec:
   case Intrinsic::amdgcn_raw_buffer_atomic_cmpswap:
   case Intrinsic::amdgcn_struct_buffer_atomic_cmpswap:
     return legalizeBufferAtomic(MI, B, IntrID);
   case Intrinsic::amdgcn_atomic_inc:
     return legalizeAtomicIncDec(MI, B, true);
   case Intrinsic::amdgcn_atomic_dec:
     return legalizeAtomicIncDec(MI, B, false);
+  case Intrinsic::trap:
+    return legalizeTrapIntrinsic(MI, MRI, B);
+  case Intrinsic::debugtrap:
+    return legalizeDebugTrapIntrinsic(MI, MRI, B);
   default: {
     if (const AMDGPU::ImageDimIntrinsicInfo *ImageDimIntr =
             AMDGPU::getImageDimIntrinsicInfo(IntrID))
       return legalizeImageIntrinsic(MI, B, Observer, ImageDimIntr);
     return true;
   }
   }
 
   return true;
 }

llvm/test/CodeGen/AMDGPU/GlobalISel/llvm.trap.ll

+; Runs original SDAG test with -global-isel
+; RUN: llc -global-isel -mtriple=amdgcn--amdhsa -verify-machineinstrs < %S/../trap.ll | FileCheck -check-prefix=GCN -check-prefix=HSA-TRAP -enable-var-scope %S/../trap.ll
+
+; RUN: llc -global-isel -mtriple=amdgcn--amdhsa -mattr=+trap-handler -verify-machineinstrs < %S/../trap.ll | FileCheck -check-prefix=GCN -check-prefix=HSA-TRAP -enable-var-scope %S/../trap.ll
+; RUN: llc -global-isel -mtriple=amdgcn--amdhsa -mattr=-trap-handler -verify-machineinstrs < %S/../trap.ll | FileCheck -check-prefix=GCN -check-prefix=NO-HSA-TRAP -enable-var-scope %S/../trap.ll
+; RUN: llc -global-isel -mtriple=amdgcn--amdhsa -mattr=-trap-handler -verify-machineinstrs < %S/../trap.ll 2>&1 | FileCheck -check-prefix=GCN -check-prefix=GCN-WARNING -enable-var-scope %S/../trap.ll
+
+; enable trap handler feature
+; RUN: llc -global-isel -mtriple=amdgcn-unknown-mesa3d -mattr=+trap-handler -verify-machineinstrs < %S/../trap.ll | FileCheck -check-prefix=GCN -check-prefix=NO-MESA-TRAP -check-prefix=TRAP-BIT -check-prefix=MESA-TRAP -enable-var-scope %S/../trap.ll
+; RUN: llc -global-isel -mtriple=amdgcn-unknown-mesa3d -mattr=+trap-handler -verify-machineinstrs < %S/../trap.ll 2>&1 | FileCheck -check-prefix=GCN -check-prefix=GCN-WARNING -check-prefix=TRAP-BIT -enable-var-scope %S/../trap.ll
+
+; disable trap handler feature
+; RUN: llc -global-isel -mtriple=amdgcn-unknown-mesa3d -mattr=-trap-handler -verify-machineinstrs < %S/../trap.ll | FileCheck -check-prefix=GCN -check-prefix=NO-MESA-TRAP -check-prefix=NO-TRAP-BIT -check-prefix=NOMESA-TRAP -enable-var-scope %S/../trap.ll
+; RUN: llc -global-isel -mtriple=amdgcn-unknown-mesa3d -mattr=-trap-handler -verify-machineinstrs < %S/../trap.ll 2>&1 | FileCheck -check-prefix=GCN -check-prefix=GCN-WARNING -check-prefix=NO-TRAP-BIT -enable-var-scope %S/../trap.ll
+
+; RUN: llc -global-isel -march=amdgcn -verify-machineinstrs < %S/../trap.ll 2>&1 | FileCheck -check-prefix=GCN -check-prefix=GCN-WARNING -enable-var-scope %S/../trap.ll