[PATCH] D153879: [AMDGPU] Handle Additional Cases in tryFoldPhiAGPR

Thu Jun 29 05:06:14 PDT 2023

arsenm accepted this revision.
arsenm added inline comments.
This revision is now accepted and ready to land.

================
Comment at: llvm/test/CodeGen/AMDGPU/fold-agpr-phis.mir:505
+  bb.2:
+    S_ENDPGM 0
+...
----------------
Pierre-vh wrote:
> arsenm wrote:
> > Pierre-vh wrote:
> > > arsenm wrote:
> > > > Could also use and end to end IR test
> > > I don't have one; I can try reducing the original app's code but that may take a while, the kernel is big.
> > > 
> > > I tried reproducing the pattern I thought was relevant using some functions from mfma_loop but I can't get it to create that two copy pattern. I will try digging a bit more, but is a end-to-end test necessary here?
> > I'd like an end to end test because eventually all this code should be deleted but we should preserve the copy-avoiding behavior. In the ideal future RegBankSelect would take care of this
> `mfma-loop` already has quite a few end-to-end testcases that will fail if this fold doesn't work, but break-large-PHIs is applied.  I can try to come up with another case that _maybe_ reproduces the two-copies pattern in the current trunk LLVM, but I can't guarantee it'll always produce that pattern in an end-to-end testcase. Changes anywhere in the pipeline may cause the lowering to be different and this new code path to no longer be hit, hence why I don't think it's particularly useful to have a end-to-end case here. I don't think it's worth the effort to come up with a testcase that may be entirely irrelevant very soon.
> 
> Now I'm thinking, something that may be useful would be to run `mfma-loop.ll` through CGP, and add that to the tests. That way if break-large-PHIs is disabled at some point in the future, we still have a stress test for AGPRs folding like SIFoldOperand does. Would you like me to add that?
> 
> Lastly, note that all of this is very specific to break-large-PHIs + DAGISel. There's a very good chance that if we switch to GISel (and no longer break PHIs in IR), that this fold will be entirely irrelevant because we won't break PHIs that way anymore, or some combine will take care of it, etc.
it doesn't really need to be guaranteed, if you can come up with something add it

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153879