[PATCH] D141557: [AMDGPU] Further reduce attaching of implicit operands to spills / copies

[Jeffrey Byrnes via Phabricator via llvm-commits]

jrbyrnes added inline comments.


================
Comment at: llvm/test/CodeGen/AMDGPU/accvgpr-copy.mir:554
     ; GFX908-NEXT: {{  $}}
-    ; GFX908-NEXT: $vgpr255 = V_MOV_B32_e32 $sgpr0, implicit $exec, implicit $sgpr0_sgpr1
+    ; GFX908-NEXT: $vgpr255 = V_MOV_B32_e32 $sgpr0, implicit $exec, implicit-def $sgpr0_sgpr1
     ; GFX908-NEXT: $agpr0 = V_ACCVGPR_WRITE_B32_e64 killed $vgpr255, implicit $exec, implicit-def $agpr0_agpr1
----------------
jrbyrnes wrote:
> arsenm wrote:
> > I don't understand what happened here, a use became a def and is breaking the value for sgpr1. I also doubt we needed this implicit use in the first place 
> The implicit-def of sgpr is used to provide def of later `V_MOV_B32_e32 $sgpr` in the case we have partially undef super sgpr. If we just use implicit-use here, then those later uses may be reschedule to a point where it is undef. This specific test doesn't have the issue. 
> 
> Currently, we just attach implicit use to every `V_MOV_B32_e32 $sgpr` which for some reason satisfies the verifier in problematic case.
> 
The verifier will be happy if 1. The subreg is not undef (i.e. we have attached implicit-def previously), or 2. The instruction using undef subreg has implicit use of super reg (https://github.com/llvm/llvm-project/blob/5073a622a785e8fd542fd15484970a435ef2e3e5/llvm/lib/CodeGen/MachineVerifier.cpp#L2485). 

Since using implicit-def breaks the value, will revert back to having implicit use on each subreg instruction. Moreover, since hazard recognizer checks MI->explicit_uses() for the uses and MI->modifiesRegister for defs, it will pick up implicit_def but not implicit use. However, post-ra scheduler will have less capability with more dependencies.


================
Comment at: llvm/test/CodeGen/AMDGPU/agpr-copy-no-free-registers.ll:651
 ; GFX908-NEXT:    s_branch .LBB3_4
+; GFX908-NEXT:  .LBB3_7: ; in Loop: Header=BB3_5 Depth=2
+; GFX908-NEXT:    s_mov_b64 s[22:23], s[14:15]
----------------
jrbyrnes wrote:
> Not sure what has happened here -- I'll look into it.
This is coming from a previous commit -- it seems the test was not updated.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141557