[llvm] r176214 - The FastISEL should be fast. But when we record statistics we use atomic operations to increment the counters.

[Jim Grosbach]

On Mar 1, 2013, at 9:13 AM, Jan Voung <jvoung at chromium.org> wrote:

> 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!
> 
> 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.

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.

> 
> 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.

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.

-Jim

> 
> Should we just stick with disabling individual stats, or should we make disabling stats more optional?
> 
> 
> 
> 
> On Thu, Feb 28, 2013 at 12:51 PM, Jim Grosbach <grosbach at apple.com> wrote:
> 
> On Feb 28, 2013, at 12:42 PM, Jan Voung <jvoung at chromium.org> wrote:
> 
>> On Thu, Feb 28, 2013 at 9:19 AM, Eli Bendersky <eliben at google.com> wrote:
>> >> I kept NumDAGBlocks and NumFastIselBlocks, because they are called once
>> >> per blocks and not once per instructions. I don't mind if we put them behind
>> >> #ifndef NDEBUG as well.
>> >
>> >
>> > It's why I was asking, I think it might be an interesting idea to put all
>> > the statistics
>> > behind NDEBUG so we can feel free to collect stats whenever rather than
>> > needing to worry about putting the high firing status under NDEBUG
>> > explicitly.
>> 
>> FWIW, I'm +1 on this. You'll still have to wrap the actual ++counter
>> code with DEBUG or some other macro-fu all over the code, though.
>> 
>> Eil
>> 
>> 
>> 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.
> 
> 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.
> 
> 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.
> 
> -Jim
> 
> <release_only_stats.patch>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20130301/4c1cc775/attachment.html>