[PATCH] D50745: [llvm-mca] Update the comments for the mca:::Stage class. NFC.

[Andrea Di Biagio via Phabricator via llvm-commits]

andreadb added a comment.

Hi Matt,

Thanks for this patch. See my comments below.

Thanks
-Andrea



================
Comment at: tools/llvm-mca/Stage.h:33
+/// The primary action of a stage is its 'execute' method, which takes as input
+/// an InstRef and can perform the stage's primary action with respect to that
+/// instruction.  That instruction is passed from stage-to-stage by the
----------------
here "primary action" is repeated in the same sentence.


================
Comment at: tools/llvm-mca/Stage.h:75
+  /// This can be used as a setup phase to prepare for any work that is
+  /// to be processed by this stage during the clock cycle.
   virtual void cycleStart() {}
----------------
s/the clock cycle/this cycle


================
Comment at: tools/llvm-mca/Stage.h:93-94
+  /// The primary action that this stage performs on an instruction (InstRef).
+  /// Returning 'Stop' prevents successor stages during this cycle from having
+  /// their 'execute' method called.  This can be called multiple times during
+  /// a clock cycle.
----------------
This is what I find particularly difficult to read. Maybe you should explain this part a bit better.
Basically, 'execute' is _only_ called on instructions that have been added to the pipeline in the current cycle.

Also (surprisingly) method 'execute' is called on every stage of the pipeline. We shouldn't be calling it if we know that a new instruction hasn't been moved to the next stage during this cycle. At the moment, it is called on every stage of the pipeline regardless of whether the new instruction has successfully transitioned to the next stage.
I find this design a bit awkward and hard to follow. We should revisit this design soon as this doesn't feel right.

"Stop" means:
  - this stage cannot process any more instructions during this cycle.

There may be several reasons why a Stage returns "Stop". The most common case is: "this stage has run out of resources, so it cannot accept new instructions during this cycle".

Add an example:
  (example: there is a limit to how many instructions can be dispatched to the schedulers every cycle. Basically, the dispatch throughput cannot be infinite, and there is a constraint on the maximum size of a dispatch group. At some point, the DispatchStage has to "stop").

"Continue" means: this stage has still resources to "execute" instruction during this same cycle. Unfortunately, it doesn't say if the instruction was successfully processed! So, this may be a bit misleading. Also, it doesn't say if the instruction is allowed to transition to the next stage. If we don't change this design, then we should early-exit from the loop (it makes no sense to call `execute(IR)` on a next stage, if IR is still stuck on a previous stage!).


https://reviews.llvm.org/D50745