[PATCH] D74688: AMDGPU/GlobalISel: Support llvm.trap and llvm.debugtrap intrinsics

[Matt Arsenault via Phabricator via llvm-commits]

arsenm added a comment.

In D74688#1879573 <https://reviews.llvm.org/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?



================
Comment at: llvm/lib/Target/AMDGPU/AMDGPULegalizerInfo.cpp:3499
+    Register DstReg(AMDGPU::SGPR0_SGPR1);
+    Register VDstReg = getLiveInRegister(MRI, DstReg, MRI.getType(DstReg));
+    MRI.setRegClass(VDstReg, &AMDGPU::SGPR_64RegClass);
----------------
loadInputValue calls getLiveInRegister, you shouldn't need it here


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPULegalizerInfo.cpp:3500
+    Register VDstReg = getLiveInRegister(MRI, DstReg, MRI.getType(DstReg));
+    MRI.setRegClass(VDstReg, &AMDGPU::SGPR_64RegClass);
+    if (!loadInputValue(VDstReg, B, Arg))
----------------
Why do you need to set the class? It shouldn't be necessary at this point


================
Comment at: llvm/test/CodeGen/AMDGPU/GlobalISel/llvm.trap.ll:95
+}
+
+attributes #0 = { nounwind noreturn }
----------------
arsenm wrote:
> arsenm wrote:
> > 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
> Actually, never mind. A non-kernel function should work. Calling it will not
Should still add a function test. Also one that has a separate, explicit use of the queue.ptr intrinsic wouldn't hurt either


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D74688/new/

https://reviews.llvm.org/D74688