[PATCH] D46907: [llvm-mca] Introduce a sequential container of Stages

[Matt Davis via Phabricator via llvm-commits]

mattd added inline comments.


================
Comment at: tools/llvm-mca/Backend.cpp:38
+Stage &Backend::getInitialStage() {
+  assert(Stages.size() > 0 && "No initial pipeline stage registered.");
+  assert(Stages[0] && "Invalid initial pipeline stage.");
----------------
andreadb wrote:
> mattd wrote:
> > This case should never occur, but since we expose this as a public api routine, I want to be super-confident that users are using the backend correctly, e.g., an initial stage is always available.
> I don't see the reason why we should expose this method. Unless I am missing something, this method is completely redundant.
> 
> If you really need it, then it should be made private. Also, the assert is not needed. Instead, you should early exit from `Backend::run()` if there are no stages to run.
> 
> As a side note: you can use method `empty()` to check if there is at least one stage in the pipeline.
> 
It's not necessary.  Eventually the backend will just loop through all of the stages calling pre/execute/post accordingly.  The backend will just go through the list sequentially ignorant of what the stage actually does.


================
Comment at: tools/llvm-mca/Backend.h:58
+  /// This container represents the stages of an instruction pipeline.
+  llvm::SmallVector<std::unique_ptr<Stage>, 2> Stages;
 
----------------
andreadb wrote:
> Any reason why a default of 2 elements? In practice we are going to have at least 4 stages.
No reason than for now I'm just trying to work in the Fetch and Dispatch portions.  The number should increase accordingly :)


================
Comment at: tools/llvm-mca/Backend.h:81
         Cycles(0) {
+    appendStage(std::move(InitialStage));
     HWS->setDispatchUnit(DU.get());
----------------
courbet wrote:
> andreadb wrote:
> > What if we initialize this stage outside of Backend.h ?
> > Ideally, the Backend should be empty to start. It shouldn't be populated by the constructor; instead, users should populate the pipeline by performing multiple calls to method appendStage(). I think that the InitialStage should be added by main in llvm-mca.cpp.
> +1. This will make it possible for subtargets to customize this more when we move to a per-subtarget factory.
Agreed!


https://reviews.llvm.org/D46907