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

[Qing Shan Zhang via Phabricator via llvm-commits]

steven.zhang added inline comments.


================
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.
----------------
fhahn wrote:
> Doc comment (///)?
typo.


================
Comment at: llvm/include/llvm/CodeGen/MachineScheduler.h:714
+  /// Save the resource state into State.
+  void save(SchedState &State) const { State = this->State; }
+
----------------
fhahn wrote:
> 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()?
Sounds good, and that is exactly what I want to do. 


================
Comment at: llvm/lib/CodeGen/MachineScheduler.cpp:1853
 
+void SchedState::reset() {
+  CurrCycle = 0;
----------------
fhahn wrote:
> 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.
Sounds good. I will leave the way how to reset/save/store the state into next NFC infrastructure patch if this one is accepted.


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

https://reviews.llvm.org/D59480