[PATCH] D101405: [AMDGPU] Change FLAT SADDR to VADDR form in moveToVALU

[Matt Arsenault via Phabricator via llvm-commits]

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