[PATCH] D154083: [AMDGPU] Rematerialize scalar loads

[Matt Arsenault via Phabricator via llvm-commits]

arsenm added inline comments.


================
Comment at: llvm/lib/Target/AMDGPU/SIInstrInfo.cpp:111
+  if (isVOP1(MI) || isVOP2(MI) || isVOP3(MI) || isSDWA(MI) || isSALU(MI) ||
+      (isSMRD(MI) && MI.isDereferenceableInvariantLoad())) {
     // Normally VALU use of exec would block the rematerialization, but that
----------------
arsenm wrote:
> piotr wrote:
> > arsenm wrote:
> > > piotr wrote:
> > > > Ironically, the addition of the check `isDereferenceableInvariantLoad` makes the examples that drive this work not handled, as the S_LOADs are not marked as `dereferenceable` at the moment (but that is another issue).
> > > Don't really need the isSMRD with the isDereferenceableInvariantLoad. We should also be able to handle any load
> > That was my intuition as well, but I added this extra check for these reasons:
> > - the negative tests in remat-smrd.mir contain cases where the loads would be rematted otherwise e.g. test_no_remat_s_load_dword_not_dereferenceable
> > - the generic hook that we bypass has a similar condition for loads (https://github.com/llvm/llvm-project/blob/c304be7cfdd2261811671feb252e31222365b475/llvm/lib/CodeGen/TargetInstrInfo.cpp#L1103)
> > 
> > Maybe checking for MMO->isInvariant() is enough? And that would mean the negative test would turn to a positive one.
> > 
> Would rematerialization ever happen in a context where the use didn't have the same dominating conditions as at the def? My intuition is that it couldn't, and thus the dereferenceable check (including the generic one) would be too strong 
As a pre-commit can you post a patch to weaken the generic check? Best to be on the same page as all targets for this


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D154083