[PATCH] D46741: [llvm-mca] Introduce a pipeline Stage class and FetchStage.

[Andrea Di Biagio via Phabricator via llvm-commits]

andreadb added a comment.

Thanks Matt,

the patch looks much better now. I like how you designed the Stage interface.
See my comments below.

In https://reviews.llvm.org/D46741#1098416, @mattd wrote:

> - The callback to Backend::eraseInstruction is still there, however, that routine now invokes the Fetch stage to remove the specified Instruction instance.  I don't like that indirection, but that would be a different patch. Specifically, once we add eventing to the Stages, then that functionality would be completely removed from the Backend.
>   - If we take the InstructionManager idea, then that would eliminate the eraseInstruction definition from Backend and any Stage.


There is no reason why you shouldn't be able to get rid of it now.
In `FetchStage::postExecute()`, you should be able to iterate over the set of instructions, and remove instructions that have reached the retire stage.



================
Comment at: tools/llvm-mca/Backend.h:89-93
+  // TODO: Move this functionality entirly into Fetch.
+  // or figure out a way to remove it entirely (via smart ptrs).
   void eraseInstruction(const InstRef &IR) {
-    Instructions.erase(IR.getSourceIndex());
+    Fetch->eraseInstruction(IR);
   }
----------------
I think that you can get rid of this, if you modify the `FetchStage::postExecute()` method.


================
Comment at: tools/llvm-mca/Stage.cpp:27
+  Listeners.insert(Listener);
+  llvm::llvm_unreachable_internal("Stage-based eventing is not implemented.");
+}
----------------
You can simply call `llvm::llvm_unreachable("...")`. 


https://reviews.llvm.org/D46741