Page MenuHomePhabricator

Add initial support for debug counting
ClosedPublic

Authored by dberlin on Feb 15 2017, 10:31 AM.

Details

Summary

(Note: I named it debug counting because that's what we called it in
GCC. I'm also mostly leaving this here until i get back to it because
i promised some folks who wanted to use it. I'll ping the review when
it's cleaned up a bit).

We have support for bisection, and bugpoint can reduce testcases
often to a single pass. But that doesn't help reduce it to a single
transform by a single pass. Which debug counting lets us do.

Debug counting lets you instrument a pass so that it only executes a
certain thing (rwhatever you want) after skipping it a certain time of
times, and then only does a certain number of executions before saying
"skip" again.

To make it concrete, for predicateinfo, if i instrument use renaming,
i can make it so it skips renaming the first N uses, renames the next
N, and then skips the rest.

This lets you narrow down a miscompilation to, often, a single
transformation, and then also debug it (by using the same command line
parameters).

Diff Detail

Repository
rL LLVM

Event Timeline

dberlin created this revision.Feb 15 2017, 10:31 AM

A few notes:

  1. I have scripts that automate binary searching the counters that i'll

upload at some point.

My current scripts only binary search a single counter, but actually, if your
miscompile requires multiple transforms, you can binary search all the
counters.

  1. Not sure what to do about counter registration.

It's easy for passes, i think.
For utilities, right now you have to just create a static object that calls registerCounter.

Right now, nothing prints the list of counters.

I could set up some macros like we have STATISTIC to register them, if folks think that is the right approach.

mehdi_amini edited edge metadata.Feb 15 2017, 11:21 AM

A few notes:

  1. I have scripts that automate binary searching the counters that i'll upload at some point.

llvm/utils/bisect ?

A few notes:

  1. I have scripts that automate binary searching the counters that i'll upload at some point.
llvm/utils/bisect  ?

Right, it's a variant of that that

  1. Knows how to format the counters
  2. Binary searches the skip, then the count.
  • Add my current hacked up script to bisect skip and count
  • Update to be cleaner
davide edited edge metadata.Feb 17 2017, 3:19 PM

When I saw this GCC feature I was really jealous =) Thanks for working on this, reviewing it now.

davide requested changes to this revision.Feb 17 2017, 3:28 PM
davide added inline comments.
include/llvm/Support/DebugCounter.h
70–71 ↗(On Diff #88911)

Add a diagnostic? ("enable debug if you want counters!") or something?

79–80 ↗(On Diff #88911)

Can counter ever go below zero?

lib/Support/DebugCounter.cpp
72–74 ↗(On Diff #88911)

At least can we output something to stderr, here, I think

This revision now requires changes to proceed.Feb 17 2017, 3:28 PM
dberlin added inline comments.Feb 17 2017, 3:32 PM
include/llvm/Support/DebugCounter.h
70–71 ↗(On Diff #88911)

We can't, really.
Because the shouldExecute call happens all the time, whether you are in debug mode or not. It just does nothing with debugging off.
(or at least, that's how GCC works)

79–80 ↗(On Diff #88911)

We use -1 to say "always execute".
I'll update.

Bigcheese added inline comments.
lib/Support/DebugCounter.cpp
19 ↗(On Diff #88911)

I believe this should be:

explicit DebugCounterList(Mods &&... Ms) : Base(std::forward<Mods>(Ms)...) {}

Which is the idiomatic way to forward arguments.

dberlin edited edge metadata.
  • Update for review comments, make it print options properly and warn.
dberlin marked 4 inline comments as done.Feb 18 2017, 2:41 PM
  • Add comments
davide accepted this revision.Feb 18 2017, 3:50 PM

LGTM! (after some minor comments inline have been addressed).
I used this (yesterday) to track down an issue internally, so very happy about this feature already.
I'd commit the PredicateInfo bits separately (it would be awesome if you can actually a test for that).

include/llvm/Support/CommandLine.h
1447–1448 ↗(On Diff #89048)

Do you need all these member variables to be protected?

lib/Support/DebugCounter.cpp
25 ↗(On Diff #89048)

Add a small comment to explain the hardcoded 6.

28–30 ↗(On Diff #89048)

Same here.

32 ↗(On Diff #89048)

NumSpaces can probably be folded here, but up to you (if you think it's less readable)

50 ↗(On Diff #89048)

full stop at the end of the comment

This revision is now accepted and ready to land.Feb 18 2017, 3:50 PM

I think some documentation in the dev guide would be welcome!

I think some documentation in the dev guide would be welcome!

Yes, I agree.

dberlin marked an inline comment as done.Feb 18 2017, 4:14 PM

I'll commit the predicateinfo bit separately. I added it to the CL as a demo, as everything gets dead-stripped on Darwin if you don't register a counter somewhere.
(it took me a good 30 minutes to figure out what was going on, as the debug-counter option disappears from opt you have no counters.
I'm actually fairly surprised this is legal, but ....)

I believe i can test the predicateinfo piece pretty easily, because we can just have a test where it skips renaming a use and verify that it skips the rename.

include/llvm/Support/CommandLine.h
1447–1448 ↗(On Diff #89048)

Actually, i don't think we need any of them any more.
I'll check and revert if i can.

lib/Support/DebugCounter.cpp
25 ↗(On Diff #89048)

It appears to be the option width used in CommandLine.cpp,but it's uncommented there too.
I've at least commented where it comes from.

32 ↗(On Diff #89048)

I tried and it creates a really weird line split, so i'm going to leave it ATM.

50 ↗(On Diff #89048)

fixed

This revision was automatically updated to reflect the committed changes.