[MCA] Add -summary-view option

[lebedev.ri]

Summary

While that is indeed a quite interesting summary stat,
there are cases where it does not really add anything
other than consuming extra lines.

Declutters the output of D48190.

Should i add some standalone tests for it?

[andreadb]

Not sure what other reviewers think about this.
However, my opinion is that the summary view should never be optional in llvm-mca.
It is only a few lines, and it gives a nice overview of the run.

[courbet]

In D48209#1133429, @andreadb wrote:

Not sure what other reviewers think about this.
However, my opinion is that the summary view should never be optional in llvm-mca.
It is only a few lines, and it gives a nice overview of the run.

I mostly agree, but it does not hurt to have the option to disable it if you don;t want it to appear. Given that the patch is very small, I would be in favor of submitting.

[lebedev.ri]

Rebased, NFC.

[andreadb]

In D48209#1133445, @courbet wrote:

In D48209#1133429, @andreadb wrote:

Not sure what other reviewers think about this.
However, my opinion is that the summary view should never be optional in llvm-mca.
It is only a few lines, and it gives a nice overview of the run.

I mostly agree, but it does not hurt to have the option to disable it if you don;t want it to appear. Given that the patch is very small, I would be in favor of submitting.

Out of curiosity: what's the motivating use case for this change? Is it just for testing purposes (i.e. D48190)?

[lebedev.ri]

In D48209#1133459, @andreadb wrote:

In D48209#1133445, @courbet wrote:

In D48209#1133429, @andreadb wrote:

Not sure what other reviewers think about this.
However, my opinion is that the summary view should never be optional in llvm-mca.
It is only a few lines, and it gives a nice overview of the run.

I mostly agree, but it does not hurt to have the option to disable it if you don;t want it to appear. Given that the patch is very small, I would be in favor of submitting.

Out of curiosity: what's the motivating use case for this change? Is it just for testing purposes (i.e. D48190)?

I personally have no need to hide that view in any *normal* case,
so at this moment - yes, just for testing purposes.
It just feels a bit inconsistent for no reason, everything else is toggleable :)

[gbedwell]

test/tools/llvm-mca/X86/option-all-views-1.s

4 ↗

(On Diff #151488)

Seems like there is no need for the ALL prefix now as there is no common output to all of them. This highlights a few issues in the update script that I'll look at fixing. I would've preferred if it had previously warned that the NOREPORT prefix was redundant and that now both the NOREPORT and ALL prefixes are redundant. Lit does spot this:

error: no check strings found with prefixes 'ALL:', 'NOREPORT:'

so I'll make sure the update script does too.

To get the test to pass, we'll need to add "-allow-empty" to that FileCheck command line, and then to make it useful, manually add:

# NOREPORT-NOT: {{.}}

to the test file. The update script will leave any prefixes where it detects a -NOT alone on the assumption that you're checking for something it doesn't understand.

[lebedev.ri]

Actually make ninja check-llvm-tools-llvm-mca pass, as suggested/noted by @gbedwell

[gbedwell]

In D48209#1133474, @lebedev.ri wrote:

Actually make ninja check-llvm-tools-llvm-mca pass, as suggested/noted by @gbedwell

Thanks. Please could you also remove the ALL prefix. I'm just testing a local patch to update_mca_test_checks.py that would make this a hard error:

$ py update_mca_test_checks.py ../test/tools/llvm-mca/X86/option-all-views-1.s
Test: ../test/tools/llvm-mca/X86/option-all-views-1.s
error: unused prefixes: ['ALL']

[lebedev.ri]

Drop -check-prefix=ALL from test/tools/llvm-mca/X86/option-all-views-1.s

[andreadb]

Given that Clement (and presumably Greg) are okay with this change, then I won't oppose it. Sorry for being pedantic.

See my comment below (as well as my comment from D48190)

Thanks
-Andrea

tools/llvm-mca/llvm-mca.cpp

112–115 ↗

(On Diff #151488)

Please mark this this option as "hidden". I don't think it should appear in the default -help output.

[lebedev.ri]

Mark -summary-view as hidden.

[gbedwell]

In D48209#1133500, @andreadb wrote:

Given that Clement (and presumably Greg) are okay with this change, then I won't oppose it. Sorry for being pedantic.

I have no really strong opinion either way, but as it doesn't seem to affect the typical workflow I don't see any real reason to oppose. It's obviously odd to have a mode where the tool outputs nothing at all, but it's not like the user can easily run into that situation without explicitly requesting it. There is at least a nice consistency in having full customization control of all of the views. I guess there could be an argument to having something like "warning: no views are enabled" but I'm wary of getting too deep into the realms of bikeshedding over a minor change like this. :)

[andreadb]

LGTM.

[gbedwell]

test/tools/llvm-mca/X86/option-all-views-1.s

118 ↗

(On Diff #151494)

pendantically, please could you just put this check above the others, as next time somebody bulk regenerates the mca tests using update_mca_test_checks, it'll want to make that change.

[lebedev.ri]

test/tools/llvm-mca/X86/option-all-views-1.s

118 ↗

(On Diff #151494)

Hopefully this is what you meant, not right after the RUN lines themselves..

[lebedev.ri]

In D48209#1133533, @andreadb wrote:

LGTM.

Thank you for the review!