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

[Jan Voung]

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.

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.

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
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20130301/add79aca/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: release_only_stats.patch
Type: application/octet-stream
Size: 8170 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20130301/add79aca/attachment.obj>