[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);
+
----------------
You should use either getNamedOperand or getNamedOperandIdx, not both


================
Comment at: lib/Target/AMDGPU/SIPeepholeSDWA.cpp:930
     SDWAInst.add(*Dst);
+    DstIdx = AMDGPU::getNamedOperandIdx(SDWAOpcode, AMDGPU::OpName::sdst);
   } else {
----------------
Why is it interesting to use check sdst for this? You can't partially write the SGPR output?

I'm also not sure why the existing code treats sdst as an alternative to vdst. What about add with carry out?


================
Comment at: lib/Target/AMDGPU/SIPeepholeSDWA.cpp:1033-1034
+      Dst->isTied() &&
+      AMDGPU::getNamedOperandIdx(Opcode, AMDGPU::OpName::dst_unused) != -1) {
+    auto DstUnused = TII->getNamedOperand(MI, AMDGPU::OpName::dst_unused);
+    assert(DstUnused &&
----------------
Combine getNamedOperandIdx/getNamedOperand usage


================
Comment at: lib/Target/AMDGPU/SIPeepholeSDWA.cpp:1036
+    assert(DstUnused &&
+           DstUnused->isImm());
+    if (DstUnused->getImm() == AMDGPU::SDWA::DstUnused::UNUSED_PRESERVE) {
----------------
Th isImm assert is redundant since getImm() will do it for you


Repository:
  rL LLVM

https://reviews.llvm.org/D44364