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

[Andrea Di Biagio via Phabricator via llvm-commits]

andreadb 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.");
----------------
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.



================
Comment at: tools/llvm-mca/Backend.h:22
 #include "Scheduler.h"
+#include "llvm/ADT/SmallVector.h"
 
----------------
Is this include really needed? You implicitly get it from Dispatch.h


================
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;
 
----------------
Any reason why a default of 2 elements? In practice we are going to have at least 4 stages.


================
Comment at: tools/llvm-mca/Backend.h:81
         Cycles(0) {
+    appendStage(std::move(InitialStage));
     HWS->setDispatchUnit(DU.get());
----------------
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.


================
Comment at: tools/llvm-mca/Backend.h:90
   void addEventListener(HWEventListener *Listener);
+  Stage &getInitialStage();
   void notifyCycleBegin(unsigned Cycle);
----------------
See my comment below. I don't think this method is needed.


https://reviews.llvm.org/D46907