[PATCH] D101405: [AMDGPU] Change FLAT SADDR to VADDR form in moveToVALU
Matt Arsenault via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Thu Apr 29 19:07:03 PDT 2021
arsenm added inline comments.
================
Comment at: llvm/lib/Target/AMDGPU/SIInstrInfo.cpp:5049-5060
+ if (OldVAddrIdx == NewVAddrIdx) {
+ assert(OldVAddrIdx == NewVAddrIdx);
+ MachineOperand &NewVAddr = Inst.getOperand(NewVAddrIdx);
+ MRI.removeRegOperandFromUseList(&NewVAddr);
+ MRI.moveOperands(&NewVAddr, &SAddr, 1);
+ Inst.RemoveOperand(OldSAddrIdx);
+ MRI.removeRegOperandFromUseList(&NewVAddr);
----------------
rampitec wrote:
> arsenm wrote:
> > At this point wouldn't it be simpler to just create a fresh new instruction and delete the old one?
> Unfortunately callers expect iterator to be intact. Otherwise probably yes, although it would not be less code.
Probably should add a comment explaining why this nightmare is here
================
Comment at: llvm/lib/Target/AMDGPU/SIInstrInfo.cpp:5049-5063
+ if (OldVAddrIdx == NewVAddrIdx) {
+ assert(OldVAddrIdx == NewVAddrIdx);
+ MachineOperand &NewVAddr = Inst.getOperand(NewVAddrIdx);
+ MRI.removeRegOperandFromUseList(&NewVAddr);
+ MRI.moveOperands(&NewVAddr, &SAddr, 1);
+ Inst.RemoveOperand(OldSAddrIdx);
+ MRI.removeRegOperandFromUseList(&NewVAddr);
----------------
arsenm wrote:
> rampitec wrote:
> > arsenm wrote:
> > > At this point wouldn't it be simpler to just create a fresh new instruction and delete the old one?
> > Unfortunately callers expect iterator to be intact. Otherwise probably yes, although it would not be less code.
> Probably should add a comment explaining why this nightmare is here
I think in some contexts in SIFixSGPRCopies the original instruction is erased, but it's a bit of a mess
================
Comment at: llvm/test/CodeGen/AMDGPU/move-load-addr-to-valu.mir:35
+ %3:vgpr_32 = V_MOV_B32_e32 0, implicit $exec
+ %4:vgpr_32 = GLOBAL_LOAD_DWORD_SADDR %1, %3, 0, 0, implicit $exec
+ %2:sreg_64 = S_AND_B64 %1, 1, implicit-def $scc
----------------
Can you also add an IR test where this matters? I'm not understanding why the DAG divergence analysis would let this happen
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D101405/new/
https://reviews.llvm.org/D101405
More information about the llvm-commits
mailing list