[PATCH] D45433: [llvm-mca] Add the ability to mark regions of code for analysis (PR36875)

[Clement Courbet via Phabricator via llvm-commits]

courbet accepted this revision.
courbet added a comment.
This revision is now accepted and ready to land.

Only minor comments, LGTM otherwise.



================
Comment at: tools/llvm-mca/CodeRegion.cpp:27
+void CodeRegions::beginRegion(StringRef Description, SMLoc Loc) {
+  const CodeRegion &CurrentRegion = *Regions.back();
+  if (CurrentRegion.startLoc().isValid() && !CurrentRegion.endLoc().isValid()) {
----------------
`assert(Regions.empty() && "missing Default region");`
(for the reader)


================
Comment at: tools/llvm-mca/CodeRegion.cpp:41
+void CodeRegions::endRegion(SMLoc Loc) {
+  CodeRegion &CurrentRegion = *Regions.back();
+  if (CurrentRegion.endLoc().isValid()) {
----------------
ditto


================
Comment at: tools/llvm-mca/CodeRegion.h:9
+//===----------------------------------------------------------------------===//
+//
+//===----------------------------------------------------------------------===//
----------------
Please add a file comment.


================
Comment at: tools/llvm-mca/SourceMgr.h:27
 class SourceMgr {
-  using InstVec = std::vector<std::unique_ptr<const llvm::MCInst>>;
-  InstVec Sequence;
+  using InstVec = const std::vector<std::unique_ptr<const llvm::MCInst>>;
+  InstVec &Sequence;
----------------
I think having the const outside of the type would make semantics of `InstVec& Sequence` and `InstVec &MCInstSequence` clearer.


https://reviews.llvm.org/D45433