Page MenuHomePhabricator

[MCA] Add support for nested and overlapping region markers
ClosedPublic

Authored by andreadb on May 8 2019, 5:52 AM.

Details

Summary

This patch fixes PR41523
https://bugs.llvm.org/show_bug.cgi?id=41523

Regions can now nest/overlap provided that they have different names.
Anonymous regions cannot overlap.

Region end markers must specify the region name. The only exception is for when there is only one user-defined region; in that particular case, the region end marker doesn't need to specify a name.

Incorrect region end markers are no longer ignored. Instead, the tool reports an error and we exit with an error code.

Added test cases to verify the new diagnostic error messages.

Updated the llvm-mca docs to reflect this feature change.

Let me know if okay to commit.

Thanks,
Andrea

Diff Detail

Repository
rL LLVM

Event Timeline

andreadb created this revision.May 8 2019, 5:52 AM
mattd added a comment.May 8 2019, 9:10 PM

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

docs/CommandGuide/llvm-mca.rst
241 ↗(On Diff #198634)

s/nutliple/multiple/

tools/llvm-mca/CodeRegionGenerator.cpp
90 ↗(On Diff #198634)

Perhaps also include colon ':' as a valid delimiter here and in line 82 above.

andreadb marked 2 inline comments as done.May 9 2019, 6:12 AM

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.

docs/CommandGuide/llvm-mca.rst
241 ↗(On Diff #198634)

I will fix it. Thanks!

tools/llvm-mca/CodeRegionGenerator.cpp
90 ↗(On Diff #198634)

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.

andreadb updated this revision to Diff 198806.May 9 2019, 6:30 AM

Patch updated.

Fixed typo in the docs.

mattd accepted this revision.May 9 2019, 7:39 AM
This revision is now accepted and ready to land.May 9 2019, 7:39 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptMay 9 2019, 8:19 AM