Page MenuHomePhabricator

[MCA] Add -summary-view option
ClosedPublic

Authored by lebedev.ri on Jun 15 2018, 2:11 AM.

Details

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?

Diff Detail

Repository
rL LLVM

Event Timeline

lebedev.ri created this revision.Jun 15 2018, 2:11 AM

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.

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.

Rebased, NFC.

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)?

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 added inline comments.Jun 15 2018, 4:44 AM
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 marked an inline comment as done.

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

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']

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

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.

Mark -summary-view as hidden.

lebedev.ri marked an inline comment as done.Jun 15 2018, 6:13 AM

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 accepted this revision.Jun 15 2018, 6:24 AM

LGTM.

This revision is now accepted and ready to land.Jun 15 2018, 6:24 AM
gbedwell added inline comments.Jun 15 2018, 6:41 AM
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 added inline comments.
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..

LGTM.

Thank you for the review!

Closed by commit rL334833: [MCA] Add -summary-view option (authored by lebedevri, committed by ). · Explain WhyJun 15 2018, 7:06 AM
This revision was automatically updated to reflect the committed changes.