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

[Jeffrey Byrnes via Phabricator via llvm-commits]

jrbyrnes added a comment.

> This isn’t the right place to apply optimizations. I’ve wanted to delete all of this code.
>
> Copy lowering should be as straightforward as possible. What we have now is doing a liveness scan for each copy, which is crazy. We should either avoid this situation in the first place by applying implicit VGPR virtual register defs so the allocator  ensures one is free at this point, or optimize all the AGPR copies at once after the fact

indirectCopyToAGPR performs two optimizations when lowering copies.

1. reuse existing accvgpr_write instead of creating a new one. If we find a match, we can reuse accvgpr_write into our dest, rather than accvpgr_read the source into vgpr, then accvgpr_write that vgpr int our dest. This involves scanning to find accvgpr_writes then to check if candidate duplicate accvpgr_writes are modified.
2. allocate vgprs used in the copy via round-robin of a triplet. This is to avoid a hazard. This involves scanning via RS.

Since the goal of expand-postra-pseudos is to lower pseudos and not apply these optimizations, it makes sense to move these elsewhere. Perhaps it makes the most sense to have a pass to do pseudo expansion cleanup (i.e. handle both of these, and more if desired)? Attaching implicit virtual vgpr defs may enable 2., but I think there would be no guarantee that they would all be allocated in the desired fashion. I think this would allow us to get rid of VGPRForAGPR copy though.

Regardless, in facilitating 1., even in a separate pass, I think that having implicit-defs of agprs attached to instructions that don't define them will pose problems, and we may end up wanting the explicitlyDefines functionality.

I propose to use this patch to provide fix for relatively high priority correctness issue, and introduce explicitlyDefines. Then, in separate work, clean up the indirectCopyToAGPR (potentially via new pass). Otherwise, I can handle above concerns here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D140708