[AMDGPU][NextUseAnalysis] PR #156079 review comments addressed

Author: alex-t

llvm/lib/Target/AMDGPU/AMDGPUNextUseAnalysis.cpp

@@ -1,3 +1,6 @@
+#include "AMDGPU.h"
+#include "AMDGPUNextUseAnalysis.h"
+
 #include "llvm/ADT/DenseMap.h"
 #include "llvm/ADT/PostOrderIterator.h"
 #include "llvm/ADT/iterator_range.h"
@@ -14,47 +17,48 @@
 #include "llvm/Passes/PassPlugin.h"
 #include "llvm/Support/Timer.h"
 
-#include "AMDGPU.h"
-
-#include "AMDGPUNextUseAnalysis.h"
-
 #define DEBUG_TYPE "amdgpu-next-use"
 
 using namespace llvm;
 
-// namespace {
+// Command-line option to enable timing instrumentation
+static cl::opt<bool> EnableTimers("amdgpu-next-use-analysis-timers",
+                                  cl::desc("Enable timing for Next Use Analysis"),
+                                  cl::init(false), cl::Hidden);
+
+// Static timers for performance tracking across all analysis runs
+static llvm::TimerGroup TG("amdgpu-next-use", "AMDGPU Next Use Analysis");
+static llvm::Timer AnalyzeTimer("analyze", "Time spent in analyze()", TG);
+static llvm::Timer GetDistanceTimer("getNextUseDistance", 
+                                     "Time spent in getNextUseDistance()", TG);
 
 // Three-tier ranking system for spiller decisions
-unsigned NextUseResult::materializeForRank(int64_t stored, unsigned snapshotOffset) const {
-  int64_t Mat64 = materialize(stored, snapshotOffset);
+unsigned NextUseResult::materializeForRank(int64_t Stored, unsigned SnapshotOffset) const {
+  int64_t Mat64 = materialize(Stored, SnapshotOffset);
 
   // Tier 1: Finite distances (0 to LoopTag-1) → return as-is
   // Tier 2: Loop-exit distances (LoopTag to DeadTag-1) → map to 60000-64999 range
   // Tier 3: Dead registers (DeadTag+) → return Infinity (65535)
   if (Mat64 >= DeadTag) {
     return Infinity;  // Tier 3: Dead registers get maximum distance
-  } else if (Mat64 >= LoopTag) {
+  }
+  if (Mat64 >= LoopTag) {
     // Tier 2: Loop-exit distances get mapped to high range [60000, 64999]
     int64_t LoopRemainder = Mat64 - LoopTag;
     // Clamp the remainder to fit in available range (5000 values)
     unsigned ClampedRemainder = static_cast<unsigned>(
         std::min(LoopRemainder, static_cast<int64_t>(4999)));
     return 60000 + ClampedRemainder;
-  } else if (Mat64 <= 0) {
+  }
+  if (Mat64 <= 0) {
     return 0;  // Tier 1: Zero-distance for immediate uses
-  } else {
-    return static_cast<unsigned>(Mat64);  // Tier 1: Finite distances as-is
   }
+  return static_cast<unsigned>(Mat64);  // Tier 1: Finite distances as-is
 }
 
 
 void NextUseResult::init(const MachineFunction &MF) {
-  TG = new TimerGroup("Next Use Analysis",
-                      "Compilation Timers for Next Use Analysis");
-  T1 = new Timer("Next Use Analysis", "Time spent in analyse()", *TG);
-  T2 = new Timer("Next Use Analysis", "Time spent in computeNextUseDistance()",
-                 *TG);
-  for (auto L : LI->getLoopsInPreorder()) {
+  for (auto *L : LI->getLoopsInPreorder()) {
     SmallVector<std::pair<MachineBasicBlock *, MachineBasicBlock *>> Exiting;
     L->getExitEdges(Exiting);
     for (auto P : Exiting) {
@@ -69,32 +73,35 @@ void NextUseResult::analyze(const MachineFunction &MF) {
   // function as the analysis users are only interested in the use distances
   // relatively to the given MI or the given block end.
   DenseMap<unsigned, VRegDistances> UpwardNextUses;
-  T1->startTimer();
+  if (EnableTimers)
+    AnalyzeTimer.startTimer();
   bool Changed = true;
   while (Changed) {
     Changed = false;
-    for (auto MBB : post_order(&MF)) {
+    for (const auto *MBB : post_order(&MF)) {
       unsigned Offset = 0;
       unsigned MBBNum = MBB->getNumber();
       VRegDistances Curr, Prev;
       if (UpwardNextUses.contains(MBBNum)) {
         Prev = UpwardNextUses[MBBNum];
       }
 
-      LLVM_DEBUG(dbgs() << "\nMerging successors for "
-                        << "MBB_" << MBB->getNumber() << "." << MBB->getName()
-                        << "\n";);
+      LLVM_DEBUG({
+        dbgs() << "\nMerging successors for "
+               << "MBB_" << MBB->getNumber() << "." << MBB->getName() << "\n";
+      });
 
-      for (auto Succ : successors(MBB)) {
+      for (auto *Succ : successors(MBB)) {
         unsigned SuccNum = Succ->getNumber();
 
         if (!UpwardNextUses.contains(SuccNum))
           continue;
 
         VRegDistances SuccDist = UpwardNextUses[SuccNum];
-        LLVM_DEBUG(dbgs() << "\nMerging "
-                          << "MBB_" << Succ->getNumber() << "."
-                          << Succ->getName() << "\n");
+        LLVM_DEBUG({
+          dbgs() << "\nMerging "
+                 << "MBB_" << Succ->getNumber() << "." << Succ->getName() << "\n";
+        });
 
         // Check if the edge from MBB to Succ goes out of the Loop
         int64_t EdgeWeight = 0;
@@ -124,10 +131,12 @@ void NextUseResult::analyze(const MachineFunction &MF) {
             }
           }
         }
-        LLVM_DEBUG(dbgs() << "\nCurr:";
-                   printVregDistances(Curr /*, 0 - we're at the block bottom*/);
-                   dbgs() << "\nSucc:";
-                   printVregDistances(SuccDist, EntryOff[SuccNum], EdgeWeight));
+        LLVM_DEBUG({
+          dbgs() << "\nCurr:";
+          printVregDistances(Curr /*, 0 - we're at the block bottom*/);
+          dbgs() << "\nSucc:";
+          printVregDistances(SuccDist, EntryOff[SuccNum], EdgeWeight);
+        });
 
         // Filter out successor's PHI operands with SourceBlock != MBB
         // PHI operands are only live on their specific incoming edge
@@ -147,7 +156,10 @@ void NextUseResult::analyze(const MachineFunction &MF) {
         }
 
         Curr.merge(SuccDist, EntryOff[SuccNum], EdgeWeight);
-        LLVM_DEBUG(dbgs() << "\nCurr after merge:"; printVregDistances(Curr));
+        LLVM_DEBUG({
+          dbgs() << "\nCurr after merge:";
+          printVregDistances(Curr);
+        });
       }
 
       NextUseMap[MBBNum].Bottom = Curr;
@@ -180,12 +192,16 @@ void NextUseResult::analyze(const MachineFunction &MF) {
       // EntryOff needs the TOTAL instruction count for correct predecessor distances
       // while InstrOffset uses individual instruction offsets for materialization
 
-      LLVM_DEBUG(dbgs() << "\nFinal distances for MBB_" << MBB->getNumber()
-                        << "." << MBB->getName() << "\n";
-                 printVregDistances(Curr, Offset));
-      LLVM_DEBUG(dbgs() << "\nPrevious distances for MBB_" << MBB->getNumber()
-                        << "." << MBB->getName() << "\n";
-                 printVregDistances(Prev, Offset));
+      LLVM_DEBUG({
+        dbgs() << "\nFinal distances for MBB_" << MBB->getNumber() << "."
+               << MBB->getName() << "\n";
+        printVregDistances(Curr, Offset);
+        dbgs() << "\nPrevious distances for MBB_" << MBB->getNumber() << "."
+               << MBB->getName() << "\n";
+        printVregDistances(Prev, Offset);
+        dbgs() << "\nUsed in block:\n";
+        dumpUsedInBlock();
+      });
 
       // EntryOff -offset of the first instruction in the block top-down walk
       EntryOff[MBBNum] = Offset;
@@ -196,18 +212,21 @@ void NextUseResult::analyze(const MachineFunction &MF) {
       Changed |= Changed4MBB;
     }
   }
-  // dumpUsedInBlock();
   // Dump complete analysis results for testing
   LLVM_DEBUG(dumpAllNextUseDistances(MF));
-  T1->stopTimer();
-  LLVM_DEBUG(TG->print(llvm::errs()));
+  if (EnableTimers) {
+    AnalyzeTimer.stopTimer();
+    TG.print(llvm::errs());
+  }
 }
 
 void NextUseResult::getFromSortedRecords(
     const VRegDistances::SortedRecords &Dists, LaneBitmask Mask,
     unsigned SnapshotOffset, unsigned &D) {
-  LLVM_DEBUG(dbgs() << "Mask : [" << PrintLaneMask(Mask) << "]  "
-                    << "SnapshotOffset=" << SnapshotOffset << "\n");
+  LLVM_DEBUG({
+    dbgs() << "Mask : [" << PrintLaneMask(Mask) << "]  "
+           << "SnapshotOffset=" << SnapshotOffset << "\n";
+  });
 
   // Records are sorted by stored value in increasing order. Since all entries
   // in this snapshot share the same SnapshotOffset, ordering by stored value
@@ -240,12 +259,14 @@ NextUseResult::getSortedSubregUses(const MachineBasicBlock::iterator I,
     if (NextUseMap[MBBNum].InstrDist[&*I].contains(VMP.getVReg())) {
       VRegDistances::SortedRecords Dists =
           NextUseMap[MBBNum].InstrDist[&*I][VMP.getVReg()];
-      LLVM_DEBUG(dbgs() << "Mask : [" << PrintLaneMask(VMP.getLaneMask())
-                        << "]\n");
+      LLVM_DEBUG({
+        dbgs() << "Mask : [" << PrintLaneMask(VMP.getLaneMask()) << "]\n";
+      });
       for (auto P : reverse(Dists)) {
         LaneBitmask UseMask = P.first;
-        LLVM_DEBUG(dbgs() << "Used mask : [" << PrintLaneMask(UseMask)
-                          << "]\n");
+        LLVM_DEBUG({
+          dbgs() << "Used mask : [" << PrintLaneMask(UseMask) << "]\n";
+        });
         if ((UseMask & VMP.getLaneMask()) == UseMask) {
           Result.push_back({VMP.getVReg(), UseMask});
         }
@@ -264,8 +285,9 @@ NextUseResult::getSortedSubregUses(const MachineBasicBlock &MBB,
       NextUseMap[MBBNum].Bottom.contains(VMP.getVReg())) {
     VRegDistances::SortedRecords Dists =
         NextUseMap[MBBNum].Bottom[VMP.getVReg()];
-    LLVM_DEBUG(dbgs() << "Mask : [" << PrintLaneMask(VMP.getLaneMask())
-                      << "]\n");
+    LLVM_DEBUG({
+      dbgs() << "Mask : [" << PrintLaneMask(VMP.getLaneMask()) << "]\n";
+    });
     for (auto P : reverse(Dists)) {
       LaneBitmask UseMask = P.first;
       LLVM_DEBUG(dbgs() << "Used mask : [" << PrintLaneMask(UseMask) << "]\n");
@@ -278,18 +300,20 @@ NextUseResult::getSortedSubregUses(const MachineBasicBlock &MBB,
 }
 
 void NextUseResult::dumpUsedInBlock() {
-  LLVM_DEBUG(for (auto P
-                  : UsedInBlock) {
+  for (auto P : UsedInBlock) {
     dbgs() << "MBB_" << P.first << ":\n";
     for (auto VMP : P.second) {
       dbgs() << "[ " << printReg(VMP.getVReg()) << " : <"
              << PrintLaneMask(VMP.getLaneMask()) << "> ]\n";
     }
-  });
+  }
 }
 
 unsigned NextUseResult::getNextUseDistance(const MachineBasicBlock::iterator I,
                                            const VRegMaskPair VMP) {
+  if (EnableTimers)
+    GetDistanceTimer.startTimer();
+  
   unsigned Dist = Infinity;
   const MachineBasicBlock *MBB = I->getParent();
   unsigned MBBNum = MBB->getNumber();
@@ -304,11 +328,16 @@ unsigned NextUseResult::getNextUseDistance(const MachineBasicBlock::iterator I,
     }
   }
 
+  if (EnableTimers)
+    GetDistanceTimer.stopTimer();
   return Dist;
 }
 
 unsigned NextUseResult::getNextUseDistance(const MachineBasicBlock &MBB,
                                            const VRegMaskPair VMP) {
+  if (EnableTimers)
+    GetDistanceTimer.startTimer();
+  
   unsigned Dist = Infinity;
   unsigned MBBNum = MBB.getNumber();
   if (NextUseMap.contains(MBBNum)) {
@@ -317,6 +346,9 @@ unsigned NextUseResult::getNextUseDistance(const MachineBasicBlock &MBB,
                            VMP.getLaneMask(), 0, Dist);
     }
   }
+  
+  if (EnableTimers)
+    GetDistanceTimer.stopTimer();
   return Dist;
 }
 
@@ -330,19 +362,6 @@ AMDGPUNextUseAnalysis::run(MachineFunction &MF,
 
 AnalysisKey AMDGPUNextUseAnalysis::Key;
 
-//} // namespace
-
-extern "C" LLVM_ATTRIBUTE_WEAK ::llvm::PassPluginLibraryInfo
-llvmGetPassPluginInfo() {
-  return {LLVM_PLUGIN_API_VERSION, "AMDGPUNextUseAnalysisPass",
-          LLVM_VERSION_STRING, [](PassBuilder &PB) {
-            PB.registerAnalysisRegistrationCallback(
-                [](MachineFunctionAnalysisManager &MFAM) {
-                  MFAM.registerPass([] { return AMDGPUNextUseAnalysis(); });
-                });
-          }};
-}
-
 char AMDGPUNextUseAnalysisWrapper::ID = 0;
 char &llvm::AMDGPUNextUseAnalysisID = AMDGPUNextUseAnalysisWrapper::ID;
 INITIALIZE_PASS_BEGIN(AMDGPUNextUseAnalysisWrapper, "amdgpu-next-use",