This is an archive of the discontinued LLVM Phabricator instance.

[MCA] Moving View.h and View.cpp from /tools/llvm-mca/Views/ to /lib/MCA/ and /include/llvm/MCA/.
ClosedPublic

Authored by holland11 on Aug 22 2021, 10:33 AM.

Details

Summary

The motivation for this change is to allow for target specific View classes that require backend (/lib/Target/) functionality. I have a downstream target that requires this so hopefully my explanation will do a good enough job motivating the change since I don't have an upstream example to include with this patch.

Most of this patch is just moving View.h and View.cpp, and changing the #include lines from files that need to include them. I also added the CustomBehaviour::getCBViews() method which returns a std::vector<std::unique_ptr<View>>. This lets a target's CustomBehaviour class return a list of these custom Views within llvm-mca.cpp so that they can be passed to the PipelinePrinter.

Currently, View.cpp and View.h reside within the /tools/llvm-mca/Views/ directory. This makes sense as Views are exclusive to mca so there's no reason why they should be part of the lib. As part of my downstream target's usage of mca, we created a new View to help visualize important information that occurs during mca's simulation. This View needs to be able to use backend symbols or interact with our target's CustomBehaviour class.

Since the target specific CustomBehaviour classes were moved to /lib/Target/<TargetName>/MCA/ in a recent patch, a custom View class cannot be placed within /tools/llvm-mca/Views/ and still interact with (#include) a target's CustomBehaviour class*. This means that the custom View needs to be defined within the target lib. And since the custom View needs to inherit from View.h, View.h needs to be part of the lib.

*A View could actually #include a target's CustomBehaviour class, but this would result in an error when trying to build with shared libs on linux. This is the same issue that originally motivated moving the target specific CustomBehaviour classes out of /tools/llvm-mca/lib/<TargetName>/ and into the target's lib.

There are other ways to get around this problem on our downstream target such as taking all of the classes and functions that our custom View needs to access from our CustomBehaviour class and then exposing the functions as virtual functions (and completely defining the classes) within the base CustomBehaviour class. This would work, but would be poor design, would be prone to merge conflicts when our downstream repo merges the upstream repo, and I also believe that this change could prove to be beneficial for an upstream target in the future. I don't think it's out of the question to think that other targets may want to define their own custom Views in the future.

When discussing this potential change with Andrea, one of his main concerns was that authors of future Views may abuse the new location of View.h and View.cpp by defining the new View classes within the lib or target lib even when they don't actually need any backend functionality. To try to make this less likely to occur, I added a section to the llvm-mca docs and also added a comment explaining that you should place the custom Views within /tools/llvm-mca/Views/ and only move it to the target lib if necessary.

Diff Detail

Event Timeline

holland11 created this revision.Aug 22 2021, 10:33 AM
holland11 requested review of this revision.Aug 22 2021, 10:33 AM

Fixed a typo in one of the comments.

holland11 edited the summary of this revision. (Show Details)Aug 22 2021, 10:43 AM
andreadb added inline comments.Aug 23 2021, 8:19 AM
llvm/docs/CommandGuide/llvm-mca.rst
1000

Nice catch!

1023–1029

Maybe we should better clarify what you mean by "backend functionality" in this context.

Most views know how to query information which is available at MC layer.
For example, most views in /tools/llvm-mca/View/ know about MCSubtargetInfo, MCSchedModel and MCInstrInfo.
My point is that, views that require backend functionalities may still be defined in a target-independent way within /tools/llvm-mca/View/, because they only need to use abstractions that are common to all targets.

1031–1038

You should also explain that:

  • Default views are still normally executed,
  • CustomBehaviour views (if any) are always appended at the end of the pipeline.

Essentially, these views are not used to override the existing pipeline. These views are something "extra" always added at the end.

Ideally, we should make the pipeline a bit more customisable, and let targets customise the ordering of views in the pipeline. Some targets might prefer if custom views are added before the TimelineView, while other targets might prefer to see those printed immediately after the Summary and InstructionInfo views.
Potentially, we could have three of those methods:

  • A method that returns a vector of views to be added at the end of the pipeline.
  • A method that returns a vector of views to be added directly after the InstructionInfoView
  • A method that returns a vector of views to be added before the TimelineView.
llvm/include/llvm/MCA/CustomBehaviour.h
97

Why not just GetViews ?

It is already a method of CustomBehaviour. I am not sure that there is an advantage in adding the extra 'CB' in the name.

llvm/lib/MCA/CustomBehaviour.cpp
27–31

Se my previous comment and the suggestion about adding multiple get*** methods (if that makes sense).

Implemented Andrea's suggestions.

Thanks for the feedback. All your suggestions make a lot of sense and I went ahead and implemented them. I went with a getStartViews(), getPostInstrInfoViews(), and getEndViews() since the TimelineView comes at the end anyways. Updated the docs snippet to be more accurate to those 3 variants and also to clarify what you mentioned. Let me know if there's any other feedback that you have. Happy to continue making any modifications.

holland11 marked 5 inline comments as done.Aug 23 2021, 11:00 AM
andreadb accepted this revision.Aug 25 2021, 3:09 AM

LGTM

Thanks Patrick!

This revision is now accepted and ready to land.Aug 25 2021, 3:09 AM