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

[Vang Thao via Phabricator via llvm-commits]

vangthao marked 6 inline comments as done.
vangthao 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;
----------------
rampitec wrote:
> 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.
```
    if (HighRPLiveIns.find(Reg) == HighRPLiveIns.end())
      continue;
```
Check is perform above to see if reg is live-in or not. If it is then it must be used in this block or is a live-through.


================
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
----------------
rampitec wrote:
> 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.
If the live-through is increasing RP in this block then by rematerializing don't we decrease RP by the live-throughs that we rematerialized?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119475