[PATCH] D117482: AMDGPU: Don't clobber source register for V_SET_INACTIVE_*

[Ruiling, Song via Phabricator via llvm-commits]

ruiling added a comment.

In D117482#3271584 <https://reviews.llvm.org/D117482#3271584>, @critson wrote:

> If I understand correctly, the root issue is the Register Coalescer not being aware of the wave-level CFG?

Actually it is because the WWM register have a weird liveness:( I don't have a good idea to model it. Even with wave-level CFG, it is still hard, right?



================
Comment at: llvm/lib/Target/AMDGPU/SIInstrInfo.cpp:1876
     unsigned Exec = ST.isWave32() ? AMDGPU::EXEC_LO : AMDGPU::EXEC;
+    BuildMI(MBB, MI, DL, get(AMDGPU::V_MOV_B32_e32), MI.getOperand(0).getReg())
+        .add(MI.getOperand(1));
----------------
critson wrote:
> Perhaps we could add a comment above this e.g.
> // We remove this COPY if/when optimizations (e.g. Register Coalescer) are made aware of the wave-level CFG.
If I understand correctly, how to model WWM register liveness is still an open question even with wave-level CFG.


================
Comment at: llvm/lib/Target/AMDGPU/SIInstructions.td:183
 def V_SET_INACTIVE_B32 : VPseudoInstSI <(outs VGPR_32:$vdst),
-  (ins VGPR_32: $src, VSrc_b32:$inactive),
+  (ins VSrc_b32: $src, VSrc_b32:$inactive),
   [(set i32:$vdst, (int_amdgcn_set_inactive i32:$src, i32:$inactive))]> {
----------------
critson wrote:
> This is just tidy up?
No, this is a small optimization, we don't need the source to be in VGPR now. Both SGPR/VGPR source operand works. Without this line change, there would be one more COPY from SGPR to VGPR.


================
Comment at: llvm/lib/Target/AMDGPU/SIInstructions.td:185
   [(set i32:$vdst, (int_amdgcn_set_inactive i32:$src, i32:$inactive))]> {
-  let Constraints = "$src = $vdst";
 }
----------------
critson wrote:
> This removes the tie constraint, correct?
Yes


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D117482