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

[Mahesha S via Phabricator via llvm-commits]

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


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPULegalizerInfo.cpp:2238-2239
+
+  LiveIn = MRI.createVirtualRegister(RC);
+  MRI.addLiveIn(Reg, LiveIn);
+
----------------
arsenm wrote:
> I still don't think you should need the register class set. This should still have the type set on the virtual register 
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.


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPULegalizerInfo.cpp:2244-2247
+void AMDGPULegalizerInfo::insertLiveInCopy(MachineIRBuilder &B,
+                                           MachineRegisterInfo &MRI,
+                                           Register LiveIn,
+                                           Register Reg) const {
----------------
arsenm wrote:
> 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
taken care


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPULegalizerInfo.cpp:2315
   // 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);
 
----------------
arsenm wrote:
> This shouldn't be called twice
taken care


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPULegalizerInfo.cpp:2325-2326
 
-  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;
----------------
arsenm wrote:
> Move the assignment up to avoid the weird !() 
taken care


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPULegalizerInfo.cpp:3569
+    const ArgDescriptor *Arg;
+    if (!(Arg = getArgDescriptor(B, AMDGPUFunctionArgInfo::QUEUE_PTR)))
+      return false;
----------------
arsenm wrote:
> Ditto
taken care


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPULegalizerInfo.h:96
+  void insertLiveInCopy(MachineIRBuilder &B, MachineRegisterInfo &MRI,
+                        Register LiveIn, Register Reg) const;
+
----------------
arsenm wrote:
> Should use MCRegister for Reg to make it clear it's physical.
BuildCopy seems not work with MCRegister, hence added an assertion to ensure that *PhyReg* is indeed physical register. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74688