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

[Stanislav Mekhanoshin via Phabricator via llvm-commits]

rampitec added inline comments.


================
Comment at: llvm/lib/Target/AMDGPU/GCNSchedStrategy.cpp:736
+    MachineInstr *UseI = &*MRI.use_instr_begin(Reg);
+    if (Def->getParent() == UseI->getParent())
+      continue;
----------------
Also skip it is if the use is not in the desired region. Actually I do not see where do you check it at all.


================
Comment at: llvm/lib/Target/AMDGPU/GCNSchedStrategy.cpp:796
+
+  // If we cannot improve occupance with live-throughs then perform third check
+  // to see if rematerializing the MI that are used in this block will improve
----------------
Typo: occupance.


================
Comment at: llvm/lib/Target/AMDGPU/GCNSchedStrategy.cpp:799
+  // occupancy. Keep the modified live-in set from second check.
+  if (!ImprovedAfterSinkingLiveThrus) {
+    // Keep a list of newly rematerialized instructions so that we can easily
----------------
I do not think you really need a whole block above dealing with live-through estimations. First it is not guaranteed to help the RP if you sink a live-through. Second it is essentially the same as this block actually performing rematerialization, just less precise. Keep just rematerialization part.


================
Comment at: llvm/lib/Target/AMDGPU/GCNSchedStrategy.cpp:717
+
+    if (!SRI->isVGPRClass(MRI.getRegClass(Reg)) || !MRI.hasOneUse(Reg))
+      continue;
----------------
vangthao wrote:
> 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.
OK, but let's handle it later. Add a TODO comment.


================
Comment at: llvm/lib/Target/AMDGPU/GCNSchedStrategy.cpp:727
+
+      ++SeenSubRegDefs[Def.getOperand(0).getSubReg()];
+
----------------
vangthao wrote:
> 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?
It is the same as rematerinalization. Just skip all of them.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119475