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?
Paths
| Differential D48209
[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, Declutters the output of D48190. Should i add some standalone tests for it?
Diff Detail
Event Timelinelebedev.ri added a parent revision: D48190: [MCA][x86][NFC] Add tests for -register-file-stats, -scheduler-stats.Jun 15 2018, 2:12 AM Comment Actions Not sure what other reviewers think about this. Comment Actions
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. Comment Actions
Out of curiosity: what's the motivating use case for this change? Is it just for testing purposes (i.e. D48190)? Comment Actions
I personally have no need to hide that view in any *normal* case,
lebedev.ri marked an inline comment as done. Comment ActionsActually make ninja check-llvm-tools-llvm-mca pass, as suggested/noted by @gbedwell Comment Actions
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'] andreadb mentioned this in D48190: [MCA][x86][NFC] Add tests for -register-file-stats, -scheduler-stats.Jun 15 2018, 5:50 AM Comment ActionsGiven 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
Comment Actions
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. :) This revision is now accepted and ready to land.Jun 15 2018, 6:24 AM
lebedev.ri added inline comments.
Closed by commit rL334833: [MCA] Add -summary-view option (authored by lebedevri). · Explain WhyJun 15 2018, 7:06 AM This revision was automatically updated to reflect the committed changes.
Revision Contents
Diff 151494 test/tools/llvm-mca/X86/option-all-stats-1.s
test/tools/llvm-mca/X86/option-all-views-1.s
test/tools/llvm-mca/X86/register-file-statistics.s
test/tools/llvm-mca/X86/scheduler-queue-usage.s
tools/llvm-mca/llvm-mca.cpp
|
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:
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:
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.