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

[Matt Arsenault via Phabricator via llvm-commits]

arsenm 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);
----------------
hsmhsm wrote:
> 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.
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);
  }
```




================
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))
----------------
hsmhsm wrote:
> 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.
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74688