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

[Mahesha S via Phabricator via llvm-commits]

hsmhsm marked 3 inline comments as done.
hsmhsm added inline comments.


================
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);
----------------
arsenm wrote:
> loadInputValue calls getLiveInRegister, you shouldn't need it here
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.


================
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))
----------------
arsenm wrote:
> Why do you need to set the class? It shouldn't be necessary at this point
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.


================
Comment at: llvm/test/CodeGen/AMDGPU/GlobalISel/llvm.trap.ll:95
+}
+
+attributes #0 = { nounwind noreturn }
----------------
arsenm wrote:
> 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
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?




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74688