[PATCH] D119475: [AMDGPU] Add scheduler pass to rematerialize trivial defs
Stanislav Mekhanoshin via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Wed Feb 16 12:52:27 PST 2022
rampitec added inline comments.
================
Comment at: llvm/lib/Target/AMDGPU/GCNSchedStrategy.cpp:654
+ static_assert(LastStage == PreRARematerialize,
+ "Passes after PreRARematerialize is not supported");
+
----------------
"are not supported".
================
Comment at: llvm/lib/Target/AMDGPU/GCNSchedStrategy.cpp:847
+ break;
+ } else if (OldMI == Regions[DefI].first) {
+ Regions[DefI] =
----------------
No else after break.
================
Comment at: llvm/lib/Target/AMDGPU/GCNSchedStrategy.cpp:851
+ break;
+ } else if (OldMI == Regions[DefI].second) {
+ Regions[DefI] = std::make_pair(Regions[DefI].first, std::prev(OldMI));
----------------
No else after break.
================
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) {
----------------
vangthao wrote:
> 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().
A basic block may finish without a terminator. It will just fall through into the next block.
================
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);
----------------
vangthao wrote:
> 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.
Just record it and erase.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D119475/new/
https://reviews.llvm.org/D119475
More information about the llvm-commits
mailing list