[PATCH] D128270: [AMDGPU] New AMDGPUInsertDelayAlu pass

[Jay Foad via Phabricator via llvm-commits]

foad marked 7 inline comments as done.
foad added inline comments.


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUInsertDelayAlu.cpp:301-302
+
+    LLVM_DEBUG(dbgs() << "  State at start of BB" << MBB.getNumber() << "\n");
+    LLVM_DEBUG(State.dump(TRI));
+
----------------
arsenm wrote:
> Can merge these into one LLVM_DEBUG. Also should use printMBBReference
Done, but the single LLVM_DEBUG looks uglier to me.


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUInsertDelayAlu.cpp:313-320
+      // Ignore some more instructions that do not generate any code.
+      // FIXME: should these be marked as isMetaInstruction?
+      switch (MI.getOpcode()) {
+      case AMDGPU::SI_MASKED_UNREACHABLE:
+      case AMDGPU::SI_RETURN_TO_EPILOG:
+      case AMDGPU::WAVE_BARRIER:
+        continue;
----------------
arsenm wrote:
> This is D128313 (except for SI_RETURN_TO_EPILOG which I'm not sure about since it's a bit weird)
Can update this when that patch lands.


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUInsertDelayAlu.cpp:345-348
+            // One of the operands of the writelane is also the output operand.
+            // This creates the insertion of redundant delays. Hence, we have to
+            // ignore this operand.
+            if (MI.getOpcode() == AMDGPU::V_WRITELANE_B32 && Op.isTied())
----------------
arsenm wrote:
> Why does the opcode need special casing here? Why not every tied operand?
Because the hardware does not see a RAW dependency here:
```
  v_mov_b32 v0, 0
  v_writelane_b32 v0, s0, 0
```
At the thread level there is no dependency. The MIR instruction uses a tied read-write operand for v0, which I suppose is a wave-level representation where the read represents the lanes that are not modified.

Other instructions with tied operands (like V_MAC) represent a real read of a VGPR in all active lanes, so we do want to model a delay for them.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D128270/new/

https://reviews.llvm.org/D128270