This is a first step towards making the pipeline configurable.
Details
Diff Detail
- Repository
- rL LLVM
- Build Status
Buildable 15952 Build 15952: arc lint + arc unit
Event Timeline
Thanks Clement.
Please run clang-format on the patch. I found that the code was not correctly formatted in several places.
See my comments below.
tools/llvm-mca/Backend.h | ||
---|---|---|
85 | 80-col. | |
tools/llvm-mca/BackendStatistics.cpp | ||
24–36 | clang-format. | |
tools/llvm-mca/BackendStatistics.h | ||
122–123 | Remove this change. Alternatively, run clang-format on this file. | |
123–124 | clang-format. | |
tools/llvm-mca/Dispatch.cpp | ||
154–167 | 80-col. | |
tools/llvm-mca/HWEventListener.h | ||
24–27 | We need a better (doxygen) comment here. | |
29–55 | Could you please explain what is the long term goal? Do you plan to remove this EventType? | |
44 | Period at end of sentence. | |
tools/llvm-mca/ResourcePressureView.cpp | ||
48–50 | You don't need braces here. | |
tools/llvm-mca/ResourcePressureView.h | ||
63 | Why this change? | |
tools/llvm-mca/Scheduler.h | ||
457–458 | Is the plan to remove this dependency in the long term? |
Thanks, PTAL.
tools/llvm-mca/HWEventListener.h | ||
---|---|---|
24–27 | oops. | |
29–55 | I've given it a shot, tell me what you think. | |
tools/llvm-mca/Scheduler.h | ||
457–458 | 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). |
tools/llvm-mca/HWEventListener.h | ||
---|---|---|
30–35 | 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. | |
70 | s/simulator/backend pipeline | |
tools/llvm-mca/Scheduler.h | ||
457–458 | 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). 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. |
tools/llvm-mca/HWEventListener.h | ||
---|---|---|
30–35 |
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.
Went for GenericEventType. | |
tools/llvm-mca/Scheduler.h | ||
457–458 |
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.
In the current design, there are two things that are actually hardcoded: This patch:
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.
Exactly. And we should also reduce how much Scheduler and DispatchUnit have to know about each other. |
tools/llvm-mca/TimelineView.cpp | ||
---|---|---|
44 | Isn't this repeated at the end of the function? |
Thanks Clement!
I only have one minor nit. Otherwise, the patch looks good to me.
-Andrea
tools/llvm-mca/HWEventListener.h | ||
---|---|---|
61 | UR should be a const ArrayRef. |
80-col.