[PATCH] D44364: [AMDGPU] Fix the SDWA Peephole phase to handle src for dst:UNUSED_PRESERVE.

[Matt Arsenault via Phabricator via llvm-commits]

arsenm added inline comments.


================
Comment at: lib/Target/AMDGPU/SIPeepholeSDWA.cpp:920-921
   // Copy dst, if it is present in original then should also be present in SDWA
   MachineOperand *Dst = TII->getNamedOperand(MI, AMDGPU::OpName::vdst);
+  auto DstIdx = AMDGPU::getNamedOperandIdx(SDWAOpcode, AMDGPU::OpName::vdst);
+
----------------
mjbedy wrote:
> arsenm wrote:
> > You should use either getNamedOperand or getNamedOperandIdx, not both
> The Dst operand is for the original instruction (which in my current understanding could be for example V_MOV_B32_e32 or V_MOV_B32_sdwa) and the DstIdx is for the replacement instruction (V_MOV_B32_sdwa). Is it true that vdst will always be the same index for both _e32 and _sdwa versions?
> 
> Will TII->getNamedOperand(SDWAInst, AMDGPU::OpName::vdst) return NULL if the operand has not yet been assigned? My assumption was that it would return NULL, which is why I used an index here. I could change the existing code to use indexes as well.
> 
> Or perhaps I should have just called it SDWADstIdx to make it clearer that it's potentially not the same index as the operand pointed to by Dst.
I'm pretty sure vdst will always be 0. If that's not true we should probably fix that.

getNamedOperand doesn't do any bounds checking, it's assumed to be operating on a complete instruction


================
Comment at: lib/Target/AMDGPU/SIPeepholeSDWA.cpp:1035
+    // We also expect a vdst, since sdst can't preserve.
+    auto PreserveDstIdx = AMDGPU::getNamedOperandIdx(SDWAOpcode, AMDGPU::OpName::vdst);
+    assert(PreserveDstIdx != -1);
----------------
mjbedy wrote:
> I couldn't find a way to get at the tied operand without using an index, but I changed all the other cases to use the non-index form.
I think the dst_preserve tied SDWA operands are manually added on (although it would be better if we had static instruction definitions for these variants, which could have a named operand)


Repository:
  rL LLVM

https://reviews.llvm.org/D44364