This is an archive of the discontinued LLVM Phabricator instance.

Add support to track total counts for DebugCounter
ClosedPublic

Authored by zhizhouy on Jul 19 2018, 10:46 AM.

Details

Summary

This patch introduces a new member Count to DebugCounter so that it now can track the total number of execution for the Counter.

Adding this feature will help us to do automated transformation level bisecting better using DebugCounter.

One (only) use case in NewGVN.cpp is also modified since we changed related functions.

We are planning to add an actual printing for this info in a follow-up.

Diff Detail

Repository
rL LLVM

Event Timeline

zhizhouy created this revision.Jul 19 2018, 10:46 AM

Thanks for this!

Looks like these were added in D29998, so I'm tagging davide, who reviewed that, in case there's context/history I'm missing.

include/llvm/Support/DebugCounter.h
81 ↗(On Diff #156316)

nit: This comment sort of feels redundant to me.

93 ↗(On Diff #156316)

Should this be >= Count?

e.g. if I ask for skip=1 + stop-after=1, we'll end up having Count == 2 on the second call to shouldExecute (which should return true).

94 ↗(On Diff #156316)

nit: this can be folded into return CounterInfo.StopAfter + CounterInfo.Skip > CounterInfo.Count;

153 ↗(On Diff #156316)

nit: can this just be Counters[Result] = {};?

162 ↗(On Diff #156316)

I don't know the history of this code, but it seems reasonable to me to make this a vector, add a string Desc to CounterInfo, and drop CounterDesc entirely. CounterVector just returns [1..UINT_MAX], in order, on insertion anyway.

(Alternatively, it seems that we could replace all three of these containers with a single StringMap<CounterInfo> and hand CounterInfo *s out instead of unsigneds, but that starts to stray pretty far from the goal of this patch, so... :) )

Can you add a unittest for the new functionality?

include/llvm/Support/DebugCounter.h
158–160 ↗(On Diff #156316)

int64_t ?

zhizhouy updated this revision to Diff 156597.Jul 20 2018, 1:38 PM
zhizhouy marked 5 inline comments as done.

Added a unit test for the change.

Tested with it and also three existing regression tests for debugcounter in test/Other.

zhizhouy added inline comments.Jul 20 2018, 1:39 PM
include/llvm/Support/DebugCounter.h
162 ↗(On Diff #156316)

I have moved Desc into the CounterInfo struct. Using vector to replace DenseMap may cause some ambiguity from UniqueVector on index. So I am keeping the DenseMap for now. We may come up with another patch to do the second change you mentioned.

include/llvm/Support/DebugCounter.h
162 ↗(On Diff #156316)

This WFM, but I don't quite understand what you mean by

Using vector to replace DenseMap may cause some ambiguity from UniqueVector on index

For my curiosity, can you elaborate a bit, please? :)

unittests/Support/DebugCounterTest.cpp
1 ↗(On Diff #156597)

Is this 80 column aligned?

16 ↗(On Diff #156597)

We probably want to put this under the NDEBUG guard, as well.

Otherwise, we'll have an unused static, which will probably cause compiler warnings.

zhizhouy updated this revision to Diff 156606.Jul 20 2018, 2:20 PM
zhizhouy marked 2 inline comments as done.
zhizhouy added inline comments.
include/llvm/Support/DebugCounter.h
162 ↗(On Diff #156316)

So in DebugCounter::print() and getCounterInfo(), ID for RegisteredCounters and Counters comes up together. So no matter we return ID or ID-1, we need to +/- 1 or one of them.

This LGTM; thanks again for the patch. Will land it for you once davide's happy

davide accepted this revision.Jul 20 2018, 2:55 PM
This revision is now accepted and ready to land.Jul 20 2018, 2:55 PM

Running ninja check-llvm after applying this on top of r337631, I see many test failures in CodeGen/AMDGPU. Are you seeing these, as well?

Running ninja check-llvm after applying this on top of r337631, I see many test failures in CodeGen/AMDGPU. Are you seeing these, as well?

Yes, I see them as well. Had a glance in the tests but could not find related code. Will investigate into it.

zhizhouy updated this revision to Diff 156655.Jul 20 2018, 6:19 PM

Fix CodeGen/AMDGPU test failures, by adding Counters only when skip/count is set.
I believe this will save many memory when skip/count is not set, thus fixing the problem.

As a result, I also reverted the changes to make CounterDesc part of CounterInfo.

Tested with ninja check-llvm and ninja check-llvm-unit.

As a result, I also reverted the changes to make CounterDesc part of CounterInfo.

Sorry, I'm confused. Is the goal here to ultimately print all of the counts? I don't see how that can happen if we're only Counting for things which have skip or stopAfter specified on the opt line.

this will save many memory when skip/count is not set, thus fixing the problem

I'm assuming you meant that the memory savings is a nice side-effect, and not the cause of the problem (which appears to be that we're now always saying that these counters are set). If not, +~300 bytes of heap crashing a test is scary. :)

zhizhouy updated this revision to Diff 156859.Jul 23 2018, 1:19 PM

Fixed the failure issue.

The tests failures are actually caused by isCounterSet(). This version keeps the behavior of the function by checking if command line option is set or not.

This revision was automatically updated to reflect the committed changes.