[PATCH] D119475: [AMDGPU] Add scheduler pass to rematerialize trivial defs

[Vang Thao via Phabricator via llvm-commits]

vangthao marked 10 inline comments as done.
vangthao added inline comments.


================
Comment at: llvm/lib/Target/AMDGPU/GCNSchedStrategy.cpp:640
+      if (Stage == PreRARematerialize) {
+        if (RegionsWithMinOcc.count() != 1 || Regions.size() == 1)
+          break;
----------------
kerbowa wrote:
> If we only try this with a single region can RegionsWithMinOcc be an Optional<int> or something? Seems like it would not be necessary to have a vector tracking all the regions that fit the criteria.
Originally I wanted to handle multiple regions, but changed it to a single region to reduce complexity. In a later patch, hopefully we can handle multiple regions.


================
Comment at: llvm/lib/Target/AMDGPU/GCNSchedStrategy.cpp:652
+        // Pressure for regions inbetween the defs and region we sinked the def
+        // to. Will need to be fixed if there is another pass after this pass.
+        static_assert(LastStage == PreRARematerialize,
----------------
kerbowa wrote:
> Why is the PreRARematerialize pass different and doesn't need this info? Do we only reschedule the single region?
Yes, we only re-schedule the single high RP region. If we sink to a lower RP region, it should not have any impact on RP for that region since we are only sinking trivially rematerializable defs with no vreg uses.


================
Comment at: llvm/lib/Target/AMDGPU/GCNSchedStrategy.h:138
+
+  // Attempt to reduce RP of VGPR by sinking trivially rematerializable
+  // instructions. Returns true if we were able to sink instruction(s).
----------------
kerbowa wrote:
> Why only VGPRs? Could this be extended to handle SGPRs and AGPRs.
Yes, this could also be extended to handle SGPRs and AGPRs. I can put a TODO here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119475