[PATCH] D44309: [llvm-mca] Refactor event listeners to make the backend agnostic to event types.

[Andrea Di Biagio via Phabricator via llvm-commits]

andreadb added inline comments.


================
Comment at: tools/llvm-mca/HWEventListener.h:30-35
+  // The goal is that subtarget-agnostic classes (e.g. Backend,
+  // HWInstructionEvent, ...) manipulate the event type as an opaque value.
+  // Only target-specific pipeline components (e.g. Scheduler, DispatchUnit,
+  // ...) and EventListener implementations manipulate event types directly.
+  // Of course some event types (e.g. Retired) can be shared among subtargets,
+  // just like pipeline components can.
----------------
Thanks for the description.

In my view, all the enum values in EventType should be known by all targets. Those should be all known concepts. This would minimize the impact of a redesign on the current views. Targets could then extend this with custom event types.
We should probably find a better name for this enum. Something like 'BaseEventType' or 'GenericEventType'. Also, please add a 'LastGenericEventType' enum value at the end.


================
Comment at: tools/llvm-mca/HWEventListener.h:70
+public:
+  // Generic events generated by the simulator.
   virtual void onCycleBegin(unsigned Cycle) {}
----------------
s/simulator/backend pipeline


================
Comment at: tools/llvm-mca/Scheduler.h:457-458
+
+  // The dispatch unit gets notified when instructions are executed.
+  DispatchUnit *DU;
 
----------------
courbet wrote:
> andreadb wrote:
> > Is the plan to remove this dependency in the long term?
> Not really. If we want the Backend class to be agnostic to the pipeline structure, we need inter-component dependencies to be carried by the component themselves. The pipeline creation function (in the subtarget, see https://reviews.llvm.org/D43951?id=136547#inline-384507) should be the one that does that the plugging.
> 
> However it would be better if the DispatchUnit was given in the ctor, maybe through some wrapper object (but let's do that if the need arises).
> 
Essentially - correct me if I am wrong - your plan is to make dependencies between components (i.e. pipeline stages) explicit. Each component knows who to notify/interact with (like in this case).
The advantage is that the Backend is now no longer responsible for orchestrating the execution. This allows the definition of a simpler Backend class which acts as a container of stages, and starts the execution. Components will know how to coordinate with each other (since they _explicitly_ know their position in the pipeline).

If my understanding is correct, then this design is still quite rigid, since components now explicitly know other componets of the pipeline. That being said, we can still achieve modularity by letting users define their custom 'DispatchUnit'/'Scheduler'(etc.) classes with (potentially) different dependencies.

Please could you share (i.e. add as a description to this review) your design and give us more details on how you plan to restructure this code? Modularity can be achieved in many different ways. I just want to make sure that we all agree with the final design. At the moment, it is still a bit unclear what you have in mind.


Repository:
  rL LLVM

https://reviews.llvm.org/D44309