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

[Andrea Di Biagio via Phabricator via llvm-commits]

andreadb added inline comments.


================
Comment at: llvm/docs/CommandGuide/llvm-mca.rst:1000
 instruction modeling (often by customizing data dependencies and detecting
-hazards that :program:`llvm-ma` has no way of knowing about).
+hazards that :program:`llvm-mca` has no way of knowing about).
 
----------------
Nice catch!


================
Comment at: llvm/docs/CommandGuide/llvm-mca.rst:1023-1029
+:program:`llvm-mca` comes with several Views such as the Timeline View and
+Summary View. These Views are generic and can work with most (if not all)
+targets. If you wish to add a new View to :program:`llvm-mca` and it does not
+require any backend functionality, please add it to the `/tools/llvm-mca/View/`
+directory. However, if your new View is target specific AND requires backend
+symbols or functionality, you can define it in the
+`/lib/Target/<TargetName>/MCA/` directory.
----------------
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.



================
Comment at: llvm/docs/CommandGuide/llvm-mca.rst:1031-1038
+To enable this target specific View, you will have to use this target's
+CustomBehaviour class to override the `CustomBehaviour::getCBViews()` method.
+This method returns a vector of Views so you will want to return a vector
+containing all of the target specific Views for the target in question.
+
+Because these target specific (and backend dependent) Views require the
+`CustomBehaviour::getCBViews()` method, these Views will not be enabled if
----------------
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.


================
Comment at: llvm/include/llvm/MCA/CustomBehaviour.h:97
+  virtual std::vector<std::unique_ptr<View>>
+  getCBViews(llvm::MCInstPrinter &IP, llvm::ArrayRef<llvm::MCInst> Insts);
 };
----------------
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.


================
Comment at: llvm/lib/MCA/CustomBehaviour.cpp:27-31
+std::vector<std::unique_ptr<View>>
+CustomBehaviour::getCBViews(llvm::MCInstPrinter &IP,
+                            llvm::ArrayRef<llvm::MCInst> Insts) {
+  return std::vector<std::unique_ptr<View>>();
+}
----------------
Se my previous comment and the suggestion about adding multiple get*** methods (if that makes sense).


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D108520/new/

https://reviews.llvm.org/D108520