[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:2238-2239
+
+  LiveIn = MRI.createVirtualRegister(RC);
+  MRI.addLiveIn(Reg, LiveIn);
+
----------------
I still don't think you should need the register class set. This should still have the type set on the virtual register 


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


================
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);
 
----------------
This shouldn't be called twice


================
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;
----------------
Move the assignment up to avoid the weird !() 


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


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPULegalizerInfo.cpp:3576
+      return false;
+    B.buildCopy(DstReg, LiveIn);
+    B.buildInstr(AMDGPU::S_TRAP)
----------------
Remove DstReg and just directly refer to the physical register


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPULegalizerInfo.h:96
+  void insertLiveInCopy(MachineIRBuilder &B, MachineRegisterInfo &MRI,
+                        Register LiveIn, Register Reg) const;
+
----------------
Should use MCRegister for Reg to make it clear it's physical.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74688