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

[Andrea Di Biagio via Phabricator via llvm-commits]

andreadb added a comment.

Hi Matt,

Thanks for the review.



================
Comment at: tools/llvm-mca/Context.cpp:43
   // Create the pipeline and its stages.
   auto P = llvm::make_unique<Pipeline>();
+  auto Fetch = llvm::make_unique<FetchStage>(IB, SrcMgr);
----------------
I plan to change that variable name to "StagePipeline".
Just to be consistent with the other naming convention.


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


================
Comment at: tools/llvm-mca/Stage.h:58
+  void setNextInSequence(Stage *NextStage) {
+    NextInSequence = NextStage;
+  }
----------------
mattd wrote:
> Would there ever be the case where a stage might want to overwrite its NextInSequence? If not, then perhaps we should add an assertion.
In future, we may want to allow stages to be inserted at specific positions (i.e. not just appended at the end of the pipeline).
That being said, for now it makes sense to have an assert. I will do it.


================
Comment at: tools/llvm-mca/Stage.h:65
+
+  /// Called when an instruction is ready to move the next stage of the
+  /// pipeline. 
----------------
mattd wrote:
> 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. 
I will elaborate this a bit more.


https://reviews.llvm.org/D50849