[PATCH] D122501: [AMDGPU] Enable PreRARematerialize scheduling pass with multiple high RP regions

[Vang Thao via Phabricator via llvm-commits]

vangthao added inline comments.


================
Comment at: llvm/lib/Target/AMDGPU/GCNSchedStrategy.cpp:739
 
     MachineInstr *UseI = &*MRI.use_instr_begin(Reg);
     if (Def->getParent() == UseI->getParent())
----------------
rampitec wrote:
> This also needs a nodbg use (use_nodbg_instructions).
If there is a debug use before the first nondbg use, this will sink the MI after the debug use.


================
Comment at: llvm/lib/Target/AMDGPU/GCNSchedStrategy.cpp:753
+        RematerializableInsts[I][Def] = UseI;
+        RematDefToLiveInRegions[Def].push_back(I);
+      }
----------------
rampitec wrote:
> It has one use, so you can break from the loop here.
We also want to collect the def if it is live-through. If we only collect uses, we may skip a trivially rematerializable def that is used in a low RP region but is increasing RP for a high RP region due to being live-through.


================
Comment at: llvm/lib/Target/AMDGPU/GCNSchedStrategy.cpp:870
+      }
+
+      SinkedDefs.push_back(Def);
----------------
rampitec wrote:
> Don't you need to remove Reg from NewLiveIns at this point for all regions?
Updated to also remove Reg from all regions' LiveIns set and re-calculate RP for regions where the Reg was live-in including regions not at MinOccupancy. This should keep cached Pressure and LiveIns up to date after sinking.


================
Comment at: llvm/lib/Target/AMDGPU/GCNSchedStrategy.cpp:903
       LIS->createAndComputeVirtRegInterval(Reg);
     }
+    return Improved;
----------------
rampitec wrote:
> It is probably also possible to call updateRegionBoundaries to restore the state.
I am not sure if we can call updateRegionBoundaries. If we sink two defs and both regions are now empty, it will not be possible to tell which def belonged to which region because both will point to the end of block iterator.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122501