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?
Differential D48209
[MCA] Add -summary-view option lebedev.ri on Jun 15 2018, 2:11 AM. Authored by
Details 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 TimelineComment 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,
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'] Comment Actions 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
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. :)
|