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

[Vang Thao via Phabricator via llvm-commits]

vangthao marked 2 inline comments as done.
vangthao added a comment.





================
Comment at: llvm/lib/Target/AMDGPU/GCNSchedStrategy.cpp:717
+
+    if (!SRI->isVGPRClass(MRI.getRegClass(Reg)) || !MRI.hasOneUse(Reg))
+      continue;
----------------
rampitec wrote:
> Actually it does not matter if that is a VGPR. AGPRs also affect occupancy and SGPRs before gfx10 too.
Also handling AGPRs and SGPRs may add complexity to this patch so I did not want to handle those instructions as well. This patch should bail if anything other than VGPR is causing occupancy to drop.


================
Comment at: llvm/lib/Target/AMDGPU/GCNSchedStrategy.cpp:727
+
+      ++SeenSubRegDefs[Def.getOperand(0).getSubReg()];
+
----------------
rampitec wrote:
> If there is a subreg def you can bail immediately, it is not rematerializable anyway. If there is more than one def too for that matter.
If it is not rematerializable, can we perform checks to see if it can be sink to its single use?


================
Comment at: llvm/lib/Target/AMDGPU/GCNSchedStrategy.h:115
+  // Regions that has the same occupancy as the latest MinOccupancy
+  BitVector RegionsWithMinOcc;
+
----------------
rampitec wrote:
> Isn't it the same as RegionsWithHighRP?
RegionsWithHighRP is broken since it using LLVM's RPTracker to set HasExcessPressure and RPTracker does not capture live-throughs. I noticed that there can be a huge difference of register pressure during scheduling (since RPTracker is used to make decisions) and the RP after when using GCNRegPressure.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119475