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

Patrick Holland via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Sun Aug 22 10:33:37 PDT 2021


holland11 created this revision.
holland11 added reviewers: andreadb, qcolombet.
holland11 added a project: LLVM.
Herald added subscribers: gbedwell, javed.absar, hiraditya, mgorny.
holland11 requested review of this revision.

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 them as virtual functions 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.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D108520

Files:
  llvm/docs/CommandGuide/llvm-mca.rst
  llvm/include/llvm/MCA/CustomBehaviour.h
  llvm/include/llvm/MCA/View.h
  llvm/lib/MCA/CMakeLists.txt
  llvm/lib/MCA/CustomBehaviour.cpp
  llvm/lib/MCA/View.cpp
  llvm/tools/llvm-mca/CMakeLists.txt
  llvm/tools/llvm-mca/PipelinePrinter.cpp
  llvm/tools/llvm-mca/PipelinePrinter.h
  llvm/tools/llvm-mca/Views/DispatchStatistics.h
  llvm/tools/llvm-mca/Views/InstructionView.h
  llvm/tools/llvm-mca/Views/RegisterFileStatistics.h
  llvm/tools/llvm-mca/Views/RetireControlUnitStatistics.h
  llvm/tools/llvm-mca/Views/SchedulerStatistics.h
  llvm/tools/llvm-mca/Views/SummaryView.h
  llvm/tools/llvm-mca/Views/View.cpp
  llvm/tools/llvm-mca/Views/View.h
  llvm/tools/llvm-mca/llvm-mca.cpp

-------------- next part --------------
A non-text attachment was scrubbed...
Name: D108520.367990.patch
Type: text/x-patch
Size: 12066 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20210822/66eda8b4/attachment.bin>


More information about the llvm-commits mailing list