[PATCH] D61676: [MCA] Add support for nested and overlapping region markers

[Andrea Di Biagio via Phabricator via llvm-commits]

andreadb marked 2 inline comments as done.
andreadb added a comment.

In D61676#1496007 <https://reviews.llvm.org/D61676#1496007>, @mattd wrote:

> This looks nice!  We should probably also have a test for the case where a user specifies an END before a BEGIN tag.


In that case, llvm-mca would report an error for the END directive.

Correct me if I am wrong, but basically your particular scenario is already covered by `test/tools/llvm-mca/X86/llvm-mca-markers-8.s`.



================
Comment at: docs/CommandGuide/llvm-mca.rst:241
+
+Note that nutliple anonymous regions cannot overlap. Also, overlapping regions
+cannot have the same name.
----------------
mattd wrote:
> s/nutliple/multiple/
I will fix it. Thanks!


================
Comment at: tools/llvm-mca/CodeRegionGenerator.cpp:90
+    // Skip spaces and tabs.
+    Position = Comment.find_first_not_of(" \t");
+    if (Position < Comment.size())
----------------
mattd wrote:
> Perhaps also include colon ':' as a valid delimiter here and in line 82 above.
Do you want to allow something like this?
```
LLVM-MCA-END: foo
```

In case, we can do that in a follow-up patch (if we want to allow it).
I personally think that it is not so important in practice to support that case. But I don't have a strong opinion on it.


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

https://reviews.llvm.org/D61676