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

[Mahesha S via Phabricator via llvm-commits]

hsmhsm marked 2 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:
> 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);
>   }
> ```
> 
> 
I have taken care of it. Now, before making copy to destination virtual register, we ensure that it is defined.


================
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:
> 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.
I have taken care of it.  I just needed an another version of getLiveInRegister() which works with TargetRegisterClass.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74688