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

Mon Mar 12 06:30:57 PDT 2018

courbet marked an inline comment as done.
courbet added inline comments.
Herald added a subscriber: gbedwell.

================
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.
----------------
andreadb wrote:
> 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.
> 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.

I fully agree. That's what I meant by "Of course some event types (e.g. Retired) can be shared among subtargets". Right now all the values in the enum apply to all subtargets, but as soon as the fonrtend is modelled, this won't be the case anymore (there are weird subtarget-specific things such as LCP stalls for Intel). I have made that clearer.

The point of the fixme was more about where this enum should be defined: I think eventually it should be in a base TD class that subtargets can extend. But that's not important right now.

> We should probably find a better name for this enum. 

Went for GenericEventType.

================
Comment at: tools/llvm-mca/Scheduler.h:457-458
+
+  // The dispatch unit gets notified when instructions are executed.
+  DispatchUnit *DU;

----------------
andreadb wrote:
> 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.
> 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).

Exactly.

> If my understanding is correct, then this design is still quite rigid, since components now explicitly know other components of the pipeline. 

In the current design, there are two things that are actually hardcoded:
(1) - The pipeline structure itself. This is because the `Backend` class has a list of exact components that it manipulates through their exact types (not through a `Component` of `PipelineStage` interface). If a subtarget wants to add a new component with type `MyComponent`, we need to add a `std::unique_ptr<MyComponent>` to class `Backend`, and add plumbing to let `MyComponent` communicate its state changes with listeners.
(2) - Interaction between components. This is because the interaction between `HWS` ` and `DU` happens in `Backend`. If a subtarget wants to add a component in between `HWS`  and `DU`, it has to modify classes `HWS`, `DU` and `Backend`. 

This patch:
 - Addresses the second part of (1): Components can emit new events, without changes to `Backend`.
 - Improves (2) a bit: you now only have to change HWS`  and `DU`, and not `Backend`.

I was actually considering this patch more as a refactoring (I'm not really changing anything ) than a change of design, to allow me to present a suggested design more easily in a next patch.

> That being said, we can still achieve modularity by letting users define their custom 'DispatchUnit'/'Scheduler'(etc.) classes with (potentially) different dependencies.

Exactly. And we should also reduce how much `Scheduler` and `DispatchUnit` have to know about each other.
In our approach, all a component has to know about the previous (resp. next) element in the pipeline is what type of elements it creates (resp. accepts): Instruction/Uop/InsertYourType. Nothing has to be known about what the component is or does. That means you can substitute any component for another in the piplene as long as it has the same input and output type, and you can add a component in the middle of the pipeline as long as it manipulates elements without changing their type.
Without going as far as this, the only way we can ensure that subtargets can reuse "generic" components is to abstract a bit the type of the components.

Repository:
  rL LLVM

https://reviews.llvm.org/D44309