This is an archive of the discontinued LLVM Phabricator instance.

[llvm-mca] Register listeners with stages; remove Pipeline dependency from Stage.
ClosedPublic

Authored by mattd on Jun 25 2018, 4:47 PM.

Details

Summary

This patch removes a few callbacks from Pipeline. It comes at the cost of
registering Listeners with all Stages. Not all stages need listeners or issue
callbacks, this registration is a bit redundant. However, as we build-out the
API, this redundancy can disappear.

The main purpose here is to move callback code from the Pipeline and into the
stages that actually issue those callbacks. This removes the back-pointer to
the Pipeline that was put into a few Stage subclasses.

Diff Detail

Repository
rL LLVM

Event Timeline

mattd created this revision.Jun 25 2018, 4:47 PM
courbet added inline comments.Jun 25 2018, 11:36 PM
tools/llvm-mca/DispatchStage.cpp
36 ↗(On Diff #152804)

This pattern appears quite a lot in various stages. One solution to avoid duplication would be to change the virtual void onXXXEvent(constHWXXXEvent&) callback to virtual void onEvent(const HWXXXEvent&). Then there can be a single notifyEvent function in Stage:

template <typename EventT>
void Stage::notifyEvent(const EventT &Event) const {
  for (HWEventListener *Listener : Listeners)
    Listener->onEvent(Event);
}

What do you think ?

tools/llvm-mca/Stage.h
31 ↗(On Diff #152804)

I think this should be made private and only exposed to subclasses as a const accessor:

protected:
  const std::set& getListeners() const { return Listeners; }
private:
  std::set<HWEventListener *> Listeners;
mattd updated this revision to Diff 152925.Jun 26 2018, 11:03 AM
mattd marked an inline comment as done.
mattd edited the summary of this revision. (Show Details)
  • Added a const accessor for the Stages::Listeners set.
tools/llvm-mca/DispatchStage.cpp
36 ↗(On Diff #152804)

I like the idea. However, it feels a bit more of a functional change than what the current patch is trying to achieve. I'd be happy to make this suggested change in separate patch. But if you feel otherwise, I can make the appropriate changes to this patch.

tools/llvm-mca/Stage.h
31 ↗(On Diff #152804)

I like that *done*

courbet accepted this revision.Jun 26 2018, 11:07 PM
courbet added inline comments.
tools/llvm-mca/DispatchStage.cpp
36 ↗(On Diff #152804)

Sounds good to me.

This revision is now accepted and ready to land.Jun 26 2018, 11:07 PM
andreadb accepted this revision.Jun 27 2018, 2:55 AM

Nice!

LGTM too.

-Andrea

This revision was automatically updated to reflect the committed changes.