[MachineScheduler] minor refactoring: new static function checkResourceLimit()

[jonpa]

Summary

I discovered a while back ago while reading the code in MachineScheduler.cpp that there was some duplicated code that was checking resource limits. I experimented with refactoring it, and this is my result. Does it look like an improvement I should commit?

NFC as far as I can see.

[fhahn]

Looks like a reasonable improvement to me, although I am not entirely sure if adding 2 methods to SchedBoundary is the best thing to do here, as the only thing checkResourceLimit uses from SchedBoundary is SchedModel. Just having static bool checkResourceLimit(unsigned LFactor, unsigned Count, unsigned Lat) seems slightly simpler.

include/llvm/CodeGen/MachineScheduler.h

685 ↗

(On Diff #120053)

I think a comment here would be good to avoid confusion with isResourceLimited below. Also, Lat -> Latency ?

687 ↗

(On Diff #120053)

popping that between 2 public blocks seems unnecessary. Could checkResourceLimit be moved to the public: block at the beginning of SchedBoundary? Also, what's the reasoning behind making updateIsResourceLimited the only protected function in SchedBoundary?

[jonpa]

Thanks for review, Florian. Does this look better?

[fhahn]

looks good to me, but let's see what others think ;)

[fhahn]

Also, could you add a more descriptive title than minor refactoring?

[MatzeB]

A clear "meh" from my side :)
Feel free to commit it.

lib/CodeGen/MachineScheduler.cpp

1838

• Use doxygen /// comments
• Unnecessary braces around return expression

[fhahn]

LGTM with Matthias's nits

[jonpa]

r316560.

lib/CodeGen/MachineScheduler.cpp

 // MachineSchedStrategy helpers used by GenericScheduler, GenericPostScheduler
 // and possibly other custom schedulers.
 //===----------------------------------------------------------------------===//
 
 static const unsigned InvalidCycle = ~0U;
 
 SchedBoundary::~SchedBoundary() { delete HazardRec; }
 
+// Given a Count of resource usage and a Latency value, return true if a
+// SchedBoundary becomes resource limited.
+static bool checkResourceLimit(unsigned LFactor, unsigned Count,
+                               unsigned Latency) {
+  return ((int)(Count - (Latency * LFactor)) > (int)LFactor);
+}
+
 void SchedBoundary::reset() {
   // A new HazardRec is created for each DAG and owned by SchedBoundary.
   // Destroying and reconstructing it is very expensive though. So keep
   // invalid, placeholder HazardRecs.
   if (HazardRec && HazardRec->isEnabled()) {
     delete HazardRec;
     HazardRec = nullptr;
   }
if (!HazardRec->isEnabled()) {
     for (; CurrCycle != NextCycle; ++CurrCycle) {
       if (isTop())
         HazardRec->AdvanceCycle();
       else
         HazardRec->RecedeCycle();
     }
   }
   CheckPending = true;
-  unsigned LFactor = SchedModel->getLatencyFactor();
   IsResourceLimited =
-    (int)(getCriticalCount() - (getScheduledLatency() * LFactor))
-    > (int)LFactor;
+      checkResourceLimit(SchedModel->getLatencyFactor(), getCriticalCount(),
+                         getScheduledLatency());
 
   DEBUG(dbgs() << "Cycle: " << CurrCycle << ' ' << Available.getName() << '\n');
 }
 
 void SchedBoundary::incExecutedResources(unsigned PIdx, unsigned Count) {
   ExecutedResCounts[PIdx] += Count;
   if (ExecutedResCounts[PIdx] > MaxExecutedResCount)
     MaxExecutedResCount = ExecutedResCounts[PIdx];
DEBUG(dbgs() << "  " << Available.getName()
           << " TopLatency SU(" << SU->NodeNum << ") " << TopLatency << "c\n");
   }
   if (SU->getHeight() > BotLatency) {
     BotLatency = SU->getHeight();
     DEBUG(dbgs() << "  " << Available.getName()
           << " BotLatency SU(" << SU->NodeNum << ") " << BotLatency << "c\n");
   }
   // If we stall for any reason, bump the cycle.
-  if (NextCycle > CurrCycle) {
+  if (NextCycle > CurrCycle)
     bumpCycle(NextCycle);
-  } else {
+  else
     // After updating ZoneCritResIdx and ExpectedLatency, check if we're
     // resource limited. If a stall occurred, bumpCycle does this.
-    unsigned LFactor = SchedModel->getLatencyFactor();
     IsResourceLimited =
-      (int)(getCriticalCount() - (getScheduledLatency() * LFactor))
-      > (int)LFactor;
-  }
+        checkResourceLimit(SchedModel->getLatencyFactor(), getCriticalCount(),
+                           getScheduledLatency());
+
   // Update CurrMOps after calling bumpCycle to handle stalls, since bumpCycle
   // resets CurrMOps. Loop to handle instructions with more MOps than issue in
   // one cycle.  Since we commonly reach the max MOps here, opportunistically
   // bump the cycle to avoid uselessly checking everything in the readyQ.
   CurrMOps += IncMOps;
 
   // Bump the cycle count for issue group constraints.
   // This must be done after NextCycle has been adjust for all other stalls.
RemLatency = std::max(RemLatency,
                         CurrZone.findMaxLatency(CurrZone.Pending.elements()));
 
   // Compute the critical resource outside the zone.
   unsigned OtherCritIdx = 0;
   unsigned OtherCount =
     OtherZone ? OtherZone->getOtherResourceCount(OtherCritIdx) : 0;
 
   bool OtherResLimited = false;
-  if (SchedModel->hasInstrSchedModel()) {
-    unsigned LFactor = SchedModel->getLatencyFactor();
-    OtherResLimited = (int)(OtherCount - (RemLatency * LFactor)) > (int)LFactor;
-  }
+  if (SchedModel->hasInstrSchedModel())
+    OtherResLimited = checkResourceLimit(SchedModel->getLatencyFactor(),
+                                         OtherCount, RemLatency);
+
   // Schedule aggressively for latency in PostRA mode. We don't check for
   // acyclic latency during PostRA, and highly out-of-order processors will
   // skip PostRA scheduling.
   if (!OtherResLimited) {
     if (IsPostRA || (RemLatency + CurrZone.getCurrCycle() > Rem.CriticalPath)) {
       Policy.ReduceLatency |= true;
       DEBUG(dbgs() << "  " << CurrZone.Available.getName()
             << " RemainingLatency " << RemLatency << " + "