[PATCH] D48576: [llvm-mca] Register listeners with stages; remove Pipeline dependency from Stage.

[Matt Davis via Phabricator via llvm-commits]

mattd added inline comments.


================
Comment at: tools/llvm-mca/DispatchStage.cpp:36
+
+void DispatchStage::notifyStallEvent(const HWStallEvent &Event) {
+  for (HWEventListener *Listener : Listeners)
----------------
courbet wrote:
> This pattern appears quite a lot in various stages.  One solution to avoid duplication would be to change the `virtual void onXXXEvent(constHWXXXEvent&)` callback to `virtual void onEvent(const HWXXXEvent&)`. Then there can be a single `notifyEvent` function in `Stage`:
> 
> ```
> template <typename EventT>
> void Stage::notifyEvent(const EventT &Event) const {
>   for (HWEventListener *Listener : Listeners)
>     Listener->onEvent(Event);
> }
> ```
> 
> What do you think ?
> 
> 
I like the idea. However, it feels a bit more of a functional change than what the current  patch is trying to achieve.  I'd be happy to make this suggested change in separate patch.  But if you feel otherwise, I can make the appropriate changes to this patch.


================
Comment at: tools/llvm-mca/Stage.h:31
+protected:
+  std::set<HWEventListener *> Listeners;
+
----------------
courbet wrote:
> I think this should be made private and only exposed to subclasses as a const accessor:
> 
> ```
> protected:
>   const std::set& getListeners() const { return Listeners; }
> private:
>   std::set<HWEventListener *> Listeners;
> ```
> 
I like that *done*


https://reviews.llvm.org/D48576