[PATCH] D154083: [AMDGPU] Rematerialize scalar loads

[Piotr Sobczak via Phabricator via llvm-commits]

piotr 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
----------------
piotr wrote:
> arsenm wrote:
> > 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
> Ok, will take a look.
I looked closer at that. Checked that if I modify `TargetInstrInfo::isReallyTriviallyReMaterializableGeneric` with a relaxed check (copy of MI.isDereferenceableInvariantLoad with  `&& MMO->isDereferenceable()` removed) - then that makes no difference in the existing tests.
X86 relies on further checks in that function "A load from a constant PseudoSourceValue is invariant".
So while that boosted my confidence that the generic code does not rely on "dereferenceable", I am not sure if any changes to the generic check are practical. That would be pretty ugly, as I'd have to pretty much copy `isDereferenceableInvariantLoad` only to have `isDereferenceable()` check removed.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D154083