[PATCH] D149393: [AMDGPU][IGLP] Parameterize the SchedGroup processing / linking in Solver

[Austin Kerbow via Phabricator via llvm-commits]

kerbowa added inline comments.


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUIGroupLP.cpp:260
+  // The direction in which we process the candidate SchedGroups per SU
+  Direction ProcessDirection;
+
----------------
Could this just be a boolean value?


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUIGroupLP.cpp:377
     for (auto &SG : SyncPipeline) {
+      LLVM_DEBUG(dbgs() << "Printing SchedGroups\n");
+      LLVM_DEBUG(dbgs() << "SchedGroup with SGID " << SG.getSGID()
----------------
Combine the two LLVM_DEBUG macros?


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUIGroupLP.cpp:395
   for (auto &SyncPipeline : BestPipeline) {
-    auto I = SyncPipeline.rbegin();
-    auto E = SyncPipeline.rend();
-    for (; I != E; ++I) {
-      auto &GroupA = *I;
-      for (auto J = std::next(I); J != E; ++J) {
-        auto &GroupB = *J;
+    for (int I = 0; I < (int)SyncPipeline.size(); I++) {
+      int Idx = ProcessDirection == Direction::BOTTOM_UP
----------------
Are we able to still use iterators here?
auto I = IsBottomUp ? SyncPipeline.rbegin() : SyncPipeline.begin();
auto E = IsBottomUp ? SyncPipeline.rend() : SyncPipeline.end();


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUIGroupLP.cpp:416
   bool MakePred = false;
 
+  for (int I = 0; I < (int)SyncPipeline.size(); I++) {
----------------
I don't think we should remove the comment explaining MakePred.


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUIGroupLP.cpp:510
   assert(CurrSU.second.size() >= 1);
-  auto I = CurrSU.second.rbegin();
-  auto E = CurrSU.second.rend();
-  for (; I != E; ++I) {
+  for (int I = 0; I < (int)CurrSU.second.size(); I++) {
+    int Idx = ProcessDirection == Direction::BOTTOM_UP
----------------
Just use iterators?


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUIGroupLP.cpp:651
 
-    // Since we have added the potential SchedGroups from bottom up, but
-    // traversed the DAG from top down, parse over the groups from last to
-    // first. If we fail to do this for the greedy algorithm, the solution will
-    // likely not be good in more complex cases.
-    auto I = CurrSU.second.rbegin();
-    auto E = CurrSU.second.rend();
-    for (; I != E; ++I) {
+    for (int I = 0; I < (int)CurrSU.second.size(); I++) {
+      int Idx = ProcessDirection == Direction::BOTTOM_UP
----------------
Ditto.


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUIGroupLP.cpp:757
 
+  virtual Direction getDirection() = 0;
+
----------------
Does this really need to be virtual if the derived class is just setting a flag to up/down? What special handling may we need here for different strategies beyond just setting the flag in the constructor?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149393