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

[Valery Pykhtin via Phabricator via llvm-commits]

vpykhtin added inline comments.


================
Comment at: llvm/lib/Target/AMDGPU/GCNSchedStrategy.cpp:725
+  const SIRegisterInfo *SRI = static_cast<const SIRegisterInfo *>(TRI);
+  GCNRPTracker::LiveRegSet HighRPLiveIns = LiveIns[HighRPIdx];
+  for (unsigned I = 0, E = MRI.getNumVirtRegs(); I != E; ++I) {
----------------
const GCNRPTracker::LiveRegSet &HighRPLiveIns


================
Comment at: llvm/lib/Target/AMDGPU/GCNSchedStrategy.cpp:775
+  // First check if we have enough trivially rematerializable instructions
+  int BestRegDiff = VGPRUsage - RematerializableInsts.size();
+  BestRegDiff = std::max(BestRegDiff, 0);
----------------
1. BestRegDiff is a misleading name, this is actually the number of VGPRs would left after sinking in the best case.
2. This implies that RematerializableInsts.size() is the number of 32bit VGPRs freed after sinking? I'm not sure if there are any 64bit rematerializable instructions, but what if they're eventually added?



================
Comment at: llvm/lib/Target/AMDGPU/GCNSchedStrategy.cpp:776
+  int BestRegDiff = VGPRUsage - RematerializableInsts.size();
+  BestRegDiff = std::max(BestRegDiff, 0);
+  unsigned BestScenarioOccupancy = ST.getOccupancyWithNumVGPRs(BestRegDiff);
----------------
Is there any way that VGPRUsage is less than RematerializableInsts.size()?


================
Comment at: llvm/lib/Target/AMDGPU/GCNSchedStrategy.cpp:805
+    LIS->InsertMachineInstrInMaps(*NewMI);
+    LIS->removeInterval(NewMI->getOperand(0).getReg());
+    LIS->createAndComputeVirtRegInterval(NewMI->getOperand(0).getReg());
----------------
Just Reg instead of NewMI->getOperand(0).getReg()?


================
Comment at: llvm/lib/Target/AMDGPU/GCNSchedStrategy.cpp:806
+    LIS->removeInterval(NewMI->getOperand(0).getReg());
+    LIS->createAndComputeVirtRegInterval(NewMI->getOperand(0).getReg());
+    InsertedMIToOldDef[NewMI] = Def;
----------------
ditto


================
Comment at: llvm/lib/Target/AMDGPU/GCNSchedStrategy.cpp:863
+  if (LiveInIt != MBBLiveIns.end())
+    MBBLiveIns.erase(LiveInIt);
+
----------------
just MBBLiveIns.erase(Regions[HighRPIdx].first->getParent()) - nothing will happen if it isn't in the map


================
Comment at: llvm/lib/Target/AMDGPU/GCNSchedStrategy.cpp:909
+        if (Removing)
+          Regions[I] = std::make_pair(std::next(MI), Regions[I].second);
+        else
----------------
Should this skip debug instructions? (and other places in this function too)


================
Comment at: llvm/lib/Target/AMDGPU/GCNSchedStrategy.cpp:920
+      }
+    } else if (BlockFound)
+      // Exited out of the regions for the block
----------------
It would be easier to understand if you search the first region for the BB and then do a loop until regions for other BBs will come.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119475