<html><head><meta http-equiv="Content-Type" content="text/html charset=windows-1252"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;"><br><div><div>On Mar 1, 2013, at 9:13 AM, Jan Voung <<a href="mailto:jvoung@chromium.org">jvoung@chromium.org</a>> wrote:</div><br class="Apple-interchange-newline"><blockquote type="cite"><div style="letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-size-adjust: auto; -webkit-text-stroke-width: 0px;"><div dir="ltr"><div>I gave it a try and it does look like the linker can optimize away the file-level globals as long as the inline operators are empty functions. It optimizes away even more though!</div><div><br></div><div><div>I modified llvm::PrintStatistics to print a warning message if -stats are disabled. However, it looks like llvm::PrintStatistics() is normally only ever called if the statistics module is linked in at all, and if any of the stats are actually Registered via the "init()" calls in the original operators. Since the inline operators now do nothing, nothing ever gets registered. So there is no printing. Also, the Statistic object file isn't even linked in anymore so the "-stats" commandline option isn't registered anymore. You'll get a non-helpful "Unknown command line argument..." instead of the message I'd put into PrintStatistics(). We'd need to have some sort of dummy statistic in each of the tools (llc, opt, ...) that gets registered to force linking and to get llvm::PrintStatistics called on shutdown.</div></div></div></div></blockquote><div><br></div><div>While not perfect, I'm OK with the "Unknown command line argument…" diagnostic. The situation I wanted to make sure to avoid is having -stats report misleading (zero) values for the counters.</div><br><blockquote type="cite"><div style="letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-size-adjust: auto; -webkit-text-stroke-width: 0px;"><div dir="ltr"><div><div><br></div></div><div>Another problem is that it looks like a lot of the "make check" tests use -stats. For example: test/Analysis/RegionInfo/block_sort.ll. I think there are about 80 of these tests. Do people run "make check" for Release builds instead of a Release+Asserts build? Otherwise, we'd have to rewrite each of the tests to not depend on stats, and it's not clear how to do that for all of the tests.<br></div></div></div></blockquote><div><br></div><div>Yes; however, I think we can make those tests depend on +Asserts and not run otherwise, or be XFAIL or something like that. I don't think we have to rewrite them all.</div><div><br></div><div>-Jim</div><br><blockquote type="cite"><div style="letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-size-adjust: auto; -webkit-text-stroke-width: 0px;"><div dir="ltr"><div><br></div><div>Should we just stick with disabling individual stats, or should we make disabling stats more optional?<br></div><div><br></div><div><br></div></div><div class="gmail_extra"><br><br><div class="gmail_quote">On Thu, Feb 28, 2013 at 12:51 PM, Jim Grosbach<span class="Apple-converted-space"> </span><span dir="ltr"><<a href="mailto:grosbach@apple.com" target="_blank">grosbach@apple.com</a>></span><span class="Apple-converted-space"> </span>wrote:<br><blockquote class="gmail_quote" style="margin: 0px 0px 0px 0.8ex; border-left-width: 1px; border-left-color: rgb(204, 204, 204); border-left-style: solid; padding-left: 1ex;"><div style="word-wrap: break-word;"><br><div><div><div class="h5"><div>On Feb 28, 2013, at 12:42 PM, Jan Voung <<a href="mailto:jvoung@chromium.org" target="_blank">jvoung@chromium.org</a>> wrote:</div><br><blockquote type="cite"><div style="letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px;"><div dir="ltr">On Thu, Feb 28, 2013 at 9:19 AM, Eli Bendersky<span> </span><span dir="ltr"><<a href="mailto:eliben@google.com" target="_blank">eliben@google.com</a>></span><span> </span>wrote:<br><div class="gmail_extra"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin: 0px 0px 0px 0.8ex; border-left-width: 1px; border-left-color: rgb(204, 204, 204); border-left-style: solid; padding-left: 1ex;"><div>>> I kept NumDAGBlocks and NumFastIselBlocks, because they are called once<br>>> per blocks and not once per instructions. I don't mind if we put them behind<br>>> #ifndef NDEBUG as well.<br>><br>><br>> It's why I was asking, I think it might be an interesting idea to put all<br>> the statistics<br>> behind NDEBUG so we can feel free to collect stats whenever rather than<br>> needing to worry about putting the high firing status under NDEBUG<br>> explicitly.<br><br></div>FWIW, I'm +1 on this. You'll still have to wrap the actual ++counter<br>code with DEBUG or some other macro-fu all over the code, though.<br><br>Eil<br></blockquote></div><br></div><div class="gmail_extra"><br></div><div class="gmail_extra">Perhaps we could just re-define the operator++(), operator--(), etc. in Statistics.h to only do real work "#if !defined(NDEBUG) || defined(LLVM_ENABLE_STATS)", and to be nops otherwise. Where LLVM_ENABLE_STATS would be like LLVM_ENABLE_DUMP... If that sounds reasonable, I could spin up a patch.</div></div></div></blockquote><div><br></div></div></div><div>This sounds good to me as long as we make the definitions smart enough that the linker will be able to optimize away the file-level globals, too. Having the operators be inline empty functions should be enough, I think.</div><div><br></div><div>In general, I very much like the idea of making the statistics be for debug builds only. llvm::PrintStatistics() should check NDEBUG and inform the user that stats are disabled in the case of release builds, too.</div><span class="HOEnZb"><font color="#888888"><div><br></div><div>-Jim</div></font></span></div></div></blockquote></div><br></div><span><release_only_stats.patch></span></div></blockquote></div><br></body></html>