[PATCH] D74688: AMDGPU/GlobalISel: Support llvm.trap and llvm.debugtrap intrinsics
Matt Arsenault via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Wed Feb 19 14:27:36 PST 2020
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
More information about the llvm-commits
mailing list