[PATCH] D61248: [NFC] Add the infrastructure to forward the scheduled state between MBB

[Qing Shan Zhang via Phabricator via llvm-commits]

steven.zhang added inline comments.


================
Comment at: llvm/include/llvm/CodeGen/MachineScheduler.h:234
+
+  /// Return the single pred MBB if it has.
+  virtual MachineBasicBlock *getScheduledPredMBB(MachineBasicBlock *MBB);
----------------
lei wrote:
> lei wrote:
> > Does `pred` mean `predecessor`?  I think it is best to use the full word for code documentation.
> Maybe this is more clear?
>   /// Return predecessor if MBB have only a single predecessor.
Thank you for the comments. I will update it.


================
Comment at: llvm/include/llvm/CodeGen/MachineScheduler.h:234
+
+  /// Return the single pred MBB if it has.
+  virtual MachineBasicBlock *getScheduledPredMBB(MachineBasicBlock *MBB);
----------------
steven.zhang wrote:
> lei wrote:
> > lei wrote:
> > > Does `pred` mean `predecessor`?  I think it is best to use the full word for code documentation.
> > Maybe this is more clear?
> >   /// Return predecessor if MBB have only a single predecessor.
> Thank you for the comments. I will update it.
Yes, it is. I will use the full item.


================
Comment at: llvm/include/llvm/CodeGen/MachineScheduler.h:304
   bool doMBBSchedRegionsTopDown() const override {
-    return SchedImpl->doMBBSchedRegionsTopDown();
+    return MF.getSubtarget().forwardScheduledState();
   }
----------------
fhahn wrote:
> IIUC this would drop the way to specify this via the scheduling policy and could change behavior for existing policies around.
> 
> It might make sense to have state forwarding imply top down here (`SchedImpl->doMBBSchedRegionsTopDown() || MF.getSubtarget().forwardScheduledState()`)
ok.


================
Comment at: llvm/include/llvm/CodeGen/TargetSubtargetInfo.h:197
+  /// Return true if scheduled state is forwarded between MBBs. 
+  virtual bool forwardScheduledState() const { return false; }
+
----------------
fhahn wrote:
> Is there a benefit of putting this setting in here, rather than in the scheduling policy, like similar settings? Unless I am missing something, I think it would be more suitable in the scheduling policy, where we already have similar options.
Sounds good.


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

https://reviews.llvm.org/D61248