[PATCH] D59480: [NFC] Add SchedState to allow forwarding the Scheduling state between MBB

[Florian Hahn via Phabricator via llvm-commits]

fhahn added a subscriber: jonpa.
fhahn added a comment.

On a first look, the patch looks useful! Any chance to provide the follow-up patch that uses it as well? that would be helpful to see the bigger picture. Also, are you seeing any benefits by preserving the state?



================
Comment at: llvm/include/llvm/CodeGen/MachineScheduler.h:615
 
-/// Each Scheduling boundary is associated with ready queues. It tracks the
-/// current cycle in the direction of movement, and maintains the state
-/// of "hazards" and other interlocks at the current cycle.
-class SchedBoundary {
-public:
-  /// SUnit::NodeQueueId: 0 (none), 1 (top), 2 (bot), 3 (both)
-  enum {
-    TopQID = 1,
-    BotQID = 2,
-    LogMaxQID = 2
-  };
-
-  ScheduleDAGMI *DAG = nullptr;
-  const TargetSchedModel *SchedModel = nullptr;
-  SchedRemainder *Rem = nullptr;
-
-  ReadyQueue Available;
-  ReadyQueue Pending;
-
-  ScheduleHazardRecognizer *HazardRec = nullptr;
-
-private:
-  /// True if the pending Q should be checked/updated before scheduling another
-  /// instruction.
-  bool CheckPending;
+// Each SchedState is associated with SchedBoundary. It tracks the resource
+// state in the direction of movement.
----------------
Doc comment (///)?


================
Comment at: llvm/include/llvm/CodeGen/MachineScheduler.h:620
+  void reset();
+  SchedState &operator=(const SchedState &State);
 
----------------
Why do we need an explicit assignment operator? Does the default one not work?


================
Comment at: llvm/include/llvm/CodeGen/MachineScheduler.h:714
+  /// Save the resource state into State.
+  void save(SchedState &State) const { State = this->State; }
+
----------------
Why not have getState() which returns const SchedState& and setState? This way, if we need a copy in the caller, it needs to be explicit. 

I think it would also be better to do the restore at the point where we initialize the ScheduleBoundary, rather than providing a restore helper. Maybe have SchedBoundary take a SchedState object, which defaults to SchedState()?


================
Comment at: llvm/lib/CodeGen/MachineScheduler.cpp:1853
 
+void SchedState::reset() {
+  CurrCycle = 0;
----------------
Personally, I think it would be simpler to have the constructor create a 'resetted' object. In SchedBoundary::reset(), we then can either assign a 'resetted' object or a user provided one.


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

https://reviews.llvm.org/D59480