This is an archive of the discontinued LLVM Phabricator instance.

[llvm-mca] Introduce the ExecuteStage (was originally the Scheduler class).
ClosedPublic

Authored by mattd on May 22 2018, 10:04 PM.

Diff Detail

Repository
rL LLVM

Event Timeline

mattd created this revision.May 22 2018, 10:04 PM
courbet added inline comments.May 22 2018, 11:14 PM
tools/llvm-mca/Backend.cpp
48 ↗(On Diff #148157)

How do you intend to make this independent of the actual type of the Stages in the long term (when we have a vector<std::unique_ptr<Stage>> instead of Dispatch and Execute) ?
Would it make sense to use Stage::isReady() ?

Hi Matt,

I am not convinced that it is a good idea to convert the Scheduler into a Stage.
I think the "execute stage" should just use/interact with the hardware schedulers to orchestrate the execution.
The statge would be also responsible for notifying events to listeners.

What I am trying to say is that the stage should only be responsible for the process (i.e. orchestrating the execution).
The logic that lives in Scheduler::cycleEvent() is essentially where the "process" logic is implemented. That logic (plus the event notification logic) should be moved to the stage class. The rest of the logic should still be part of the Scheduler class. Basically, the stage is only where we control the execution. It helps decoupling the actual feature from the process (i.e. how we use the feature/interact with the hardware components).

mattd updated this revision to Diff 150568.Jun 8 2018, 2:50 PM
mattd edited the summary of this revision. (Show Details)

This is an updated patch that maintains the logic of the Scheduler, but decoupling the hardware component from the execution component.
The ExecuteStage drives the Scheduler and provides the notification functionality.

  • Scheduler::cycleEvent is now ExecuteStage::preExecute
  • Scheduler::scheduleInstruction is now ExecuteStage::execute
  • Scheduler::canBeDispatched now returns a HWStallEvent (or NoStall) instead of a bool. We might want to rename this routine to something more appropriate, it makes more sense to name the routine something that does not imply a boolean return value. I left the name the same for this patch.
  • The Backend pointer (Owner) in ExecuteStage will eventually go away once we register the listeners to the stages.

Hi Matt

thanks for the patch. Sorry for the late review; it took me a while to go through all of its parts (I was not expecting so many changes).
I have a few nits (see my comments below):

tools/llvm-mca/ExecuteStage.cpp
30 ↗(On Diff #150568)

Something like a small paragraph with a brief overview of the sequence of steps performed by this method would really help. I know that it was missing in the original implementation. However, I think it really helps a lot if we add a small description here.

Some steps could be moved to a separate function. That might help with making the code more readable (if we choose good names for functions).

78 ↗(On Diff #150568)

Please add a small comment here.

tools/llvm-mca/HWEventListener.h
106–107 ↗(On Diff #150568)

See my other comment.

tools/llvm-mca/Scheduler.cpp
235–236 ↗(On Diff #150568)

Could you change this method so that it returns a bool? I don't particularly like the fact that , as a consequence of this change, we now need a new value in the GenericEventType enum.
You can make the enum an in/out argument to this method. If canBeDispatched returns false, then the HWStallEvent::GenericEventType contains the reason why dispatch failed.

tools/llvm-mca/Scheduler.h
462 ↗(On Diff #150568)

Emphasize that this method is meant to be called when a new cycle starts.
Also, add a small sentence to better describe the semantic of this method.

497–499 ↗(On Diff #150568)

You can reuse the previous NDEBUG block. I think you should remove the #endif at line 497 instead of opening a new block.

mattd added inline comments.Jun 13 2018, 10:21 AM
tools/llvm-mca/Scheduler.cpp
235–236 ↗(On Diff #150568)

Sounds good! Alternatively, we could make canBeDispatched return Optional<HWStallEvent::GenericEventType>, but I'm happy with an in/out in this case, simple enough.

mattd updated this revision to Diff 151247.Jun 13 2018, 2:16 PM
mattd marked 6 inline comments as done.

Thanks for the review @andreadb! I updated this based on your suggestions.

andreadb accepted this revision.Jun 13 2018, 4:10 PM

LGTM.

Thanks Matt!

This revision is now accepted and ready to land.Jun 13 2018, 4:10 PM
This revision was automatically updated to reflect the committed changes.