This is an archive of the discontinued LLVM Phabricator instance.

[llvm-mca] Introduce a sequential container of Stages
ClosedPublic

Authored by mattd on May 15 2018, 3:20 PM.

Details

Summary

Remove explicit stages and introduce a list of stages.

A pipeline should be composed of an arbitrary list of stages, and not any
predefined list of stages in the Backend. The Backend should not know of any
particular stage, rather it should only be concerned that it has a list of
stages, and that those stages will fulfill the contract of what it means to be
a Stage (namely pre/post/execute a given instruction).

For now, we leave the original set of stages defined in the Backend ctor;
however, I imagine these will be moved out at a later time.

This patch makes an adjustment to the semantics of Stage::isReady.
Specifically, what the Backend really needs to know is if a Stage has
unfinished work. With that said, it is more appropriately renamed
Stage::hasWorkToComplete(). This change will clean up the check in
Backend::run(), allowing us to query each stage to see if there is unfinished
work, regardless of what subclass a stage might be. I feel that this change
simplifies the semantics too, but that's a subjective statement.

Given how RetireStage and ExecuteStage handle data in their preExecute(), I've
had to change the order of Retire and Execute in our stage list. Retire must
complete any of its preExecute actions before ExecuteStage's preExecute can
take control. This is mainly because both stages utilize the RCU. In the
meantime, I want to see if I can adjust that or remove that coupling.

Diff Detail

Event Timeline

mattd created this revision.May 15 2018, 3:20 PM
mattd added inline comments.May 15 2018, 3:21 PM
tools/llvm-mca/Backend.cpp
80

This case should never occur, but since we expose this as a public api routine, I want to be super-confident that users are using the backend correctly, e.g., an initial stage is always available.

mattd edited the summary of this revision. (Show Details)May 15 2018, 3:22 PM
mattd updated this revision to Diff 146967.May 15 2018, 5:19 PM

This patch isn't crucial now, but eventually we'll most likely want to collect all stages in a sequential container.

andreadb added inline comments.May 16 2018, 1:56 AM
tools/llvm-mca/Backend.cpp
80

I don't see the reason why we should expose this method. Unless I am missing something, this method is completely redundant.

If you really need it, then it should be made private. Also, the assert is not needed. Instead, you should early exit from Backend::run() if there are no stages to run.

As a side note: you can use method empty() to check if there is at least one stage in the pipeline.

tools/llvm-mca/Backend.h
23

Is this include really needed? You implicitly get it from Dispatch.h

62

Any reason why a default of 2 elements? In practice we are going to have at least 4 stages.

78

What if we initialize this stage outside of Backend.h ?
Ideally, the Backend should be empty to start. It shouldn't be populated by the constructor; instead, users should populate the pipeline by performing multiple calls to method appendStage(). I think that the InitialStage should be added by main in llvm-mca.cpp.

81

See my comment below. I don't think this method is needed.

courbet added inline comments.May 16 2018, 8:11 AM
tools/llvm-mca/Backend.h
78

+1. This will make it possible for subtargets to customize this more when we move to a per-subtarget factory.

mattd added inline comments.May 16 2018, 8:17 AM
tools/llvm-mca/Backend.cpp
80

It's not necessary. Eventually the backend will just loop through all of the stages calling pre/execute/post accordingly. The backend will just go through the list sequentially ignorant of what the stage actually does.

tools/llvm-mca/Backend.h
62

No reason than for now I'm just trying to work in the Fetch and Dispatch portions. The number should increase accordingly :)

78

Agreed!

courbet added inline comments.May 17 2018, 5:15 AM
tools/llvm-mca/Backend.h
23

I think it should be included:

https://llvm.org/docs/CodingStandards.html#minimal-list-of-includes

You must include all of the header files that you are using
andreadb added inline comments.May 17 2018, 5:19 AM
tools/llvm-mca/Backend.h
23
[...] you can include them either directly or indirectly through another header file

I guess, it is a matter of how you interpret it?
Given the low importance of this, I don't really mind if we keep the include.

mattd updated this revision to Diff 151443.Jun 14 2018, 5:40 PM
mattd edited the summary of this revision. (Show Details)
mattd updated this revision to Diff 151937.Jun 19 2018, 9:58 AM
mattd updated this revision to Diff 152399.Jun 21 2018, 4:14 PM

This change lifts the component initialization (stages and simulated hardware bits) out of Backend and into llvm-mca. That code will eventually be exposed as initializeDefaultPipeline. This patch removes the ownership of the simulated hardware pieces (Scheduler, RegisterFile, etc) from the Backend. Now a user can create stages using the simulated hardware components without having to enforce that the Backend own those components. That happens to simplify pipeline construction; however, we'll eventually need to pass ownership of the hardware with the stages. But for now, the patch just decouples hardware, stages, and the backend.

mattd added inline comments.Jun 21 2018, 4:16 PM
tools/llvm-mca/llvm-mca.cpp
74 ↗(On Diff #152399)

I know the formatting here is not related to the primary idea behind this patch, but I decided to run clang-format on the entire file. If you prefer, I can ignore the formatting of the options here, and submit that as a NFC later.

andreadb accepted this revision.Jun 22 2018, 2:56 AM

LGTM.

Thanks for doing this!

tools/llvm-mca/llvm-mca.cpp
74 ↗(On Diff #152399)

No problem. It looks good :-)

This revision is now accepted and ready to land.Jun 22 2018, 2:56 AM
This revision was automatically updated to reflect the committed changes.