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

[Vang Thao via Phabricator via llvm-commits]

vangthao added inline comments.


================
Comment at: llvm/lib/Target/AMDGPU/GCNSchedStrategy.cpp:830
+      // not need to check RegionEnd since it can only be a call or terminator
+      // instruction and we do not touch those type of instructions.
+      for (unsigned DefI = 0, DefE = Regions.size(); DefI != DefE; ++DefI) {
----------------
rampitec wrote:
> rampitec wrote:
> > You actually need to update RegionEnd because there can be a fallthrough without a terminator.
> And actually remember that region is not the same as basic block. It may start and end in the middle of a block.
Can you give an example of a fallthrough case? I thought regions were decided by isSchedBoundary().


================
Comment at: llvm/lib/Target/AMDGPU/GCNSchedStrategy.cpp:872
+  // Update cached live-ins and register pressure after rematerializing
+  LiveIns[HighRPIdx].copyFrom(NewLiveIns);
+  GCNDownwardRPTracker RPTracker(*LIS);
----------------
rampitec wrote:
> You also need to either update MBBLiveIns and BBLiveInMap cache or erase this block from the map. Presumably it will not be needed anymore, so you can simply erase it.
BBLiveInMap seems to require the original first non-dbg instruction before scheduling as the key.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119475