[PATCH] D50849: [llvm-mca] Refactor how execution is orchestrated by the Pipeline. NFCI

[Matt Davis via Phabricator via llvm-commits]

mattd added a comment.

I made a few comments, all just nits.  This is a pretty substantial change in design, but better.  LGTM.  Thanks for this!



================
Comment at: tools/llvm-mca/FetchStage.cpp:37
+    return llvm::ErrorSuccess();
+
   const SourceRef SR = SM.peekNext();
----------------
nit: Not sure if we want whitespace here.  Lines 41 and 43 below do not use white space to separate the if block.


================
Comment at: tools/llvm-mca/Stage.h:58
+  void setNextInSequence(Stage *NextStage) {
+    NextInSequence = NextStage;
+  }
----------------
Would there ever be the case where a stage might want to overwrite its NextInSequence? If not, then perhaps we should add an assertion.


================
Comment at: tools/llvm-mca/Stage.h:65
+
+  /// Called when an instruction is ready to move the next stage of the
+  /// pipeline. 
----------------
This is the fundamental difference from this design and the previous incarnation of Stage.  I think this comment should be a bit more explicit in saying that a Stage is responsible for transferring the instruction it completes processing on to its immediate successor stage. 


https://reviews.llvm.org/D50849