[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
----------------
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 


================
Comment at: llvm/lib/Target/AMDGPU/SIInstrInfo.cpp:2354
+    MI->getOperand(0).setIsUndef(false);
+    MRI.setRegClass(DestReg, &AMDGPU::SGPR_256RegClass);
+
----------------
arsenm wrote:
> Can you go through getSubRegisterClass (possibly with getMatchingSuperRegClass and getSubClassWithSubReg) to avoid hardcoding this 
Easier yet would be just use the result class from the instruction desc 

Also, is this safe from other users with a different class?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D154083