[PATCH] D140708: [AMDGPU][GFX908] Only consider explicit defs of src reg in indirect agpr copy

[Jeffrey Byrnes via Phabricator via llvm-commits]

jrbyrnes added inline comments.


================
Comment at: llvm/lib/Target/AMDGPU/SIInstrInfo.cpp:577
     --Def;
-    if (!Def->definesRegister(SrcReg, &RI))
+    if (!Def->explicitlyDefinesRegister(SrcReg, &RI))
       continue;
----------------
arsenm wrote:
> Assuming the intent of implicit defs makes me nervous. They're there for liveness representation we don't want to just ignore. What does this solve exactly? I thought we always reserved a register for this now, so you don't need to actually scan for anything
We don't need to scan for anything -- it is just an optimization. When copying agprs on gfx908, this code is used to pick up the case where we already have an accvgpr_write that does the same thing as one we are about to create. However, it is incorrectly picking up accvgpr_writes because we have attached to spills implicit def of the entire wide agpr. 

So when lowering 
`agpr0_agpr1 = copy agpr2_agpr3, `
we will find 
`vgprx = v_accvgpr_write agpr2, implicit-def agpr2_agpr3 `
and incorrectly use `vgprx` as holding `agpr3` due to `implicit-def`.

If we don't find any such accvgpr_write, then we use a temp register to copy over. The register we use for this takes into account the reserved registers to find an unused one. The other patch was to mark vgpr used for agpr spills as reserved so the temp didn't clobber.

I think this case is niche enough to be able to assume the intent of implicit-def. With this patch we at least err on the side of caution in that we only apply optimization if the found instruction explicitly defines our reg. Furthermore, if we do find such a reg, we check instructions in between that def and our use for any modifications to this reg (including implicit defs).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D140708