On Mon, Nov 11, 2019 at 3:33 PM Martin Liška <mli...@suse.cz> wrote:
>
> On 11/11/19 3:19 PM, Richard Biener wrote:
> > On Mon, Nov 11, 2019 at 9:17 AM Martin Liška <mli...@suse.cz> wrote:
> >>
> >> Hi.
> >>
> >> The patch makes debug counter more usable. In particular, one can now
> >> list multiple closed intervals and -fdbg-cnt-list can reflect that.
> >> Based on the discussion with Richard, I decided to leave semi-closed
> >> intervals and make it closed, it's more intuitive.
> >>
> >> Example:
> >> $ g++ -O2 tramp3d-v4.ii
> >> -fdbg-cnt=match:1-2:6-10:11-20,dce:100:105-200,merged_ipa_icf:300-300 -c
> >> -fdbg-cnt-list
> >> ...
> >> counter name closed intervals
> >> -----------------------------------------------------------------
> >> ...
> >> dce [1, 100], [105, 200]
> >> ...
> >> match [1, 2], [6, 10], [11, 20]
> >> merged_ipa_icf [300, 300]
> >>
> >> Patch can bootstrap on x86_64-linux-gnu and survives regression tests.
> >>
> >> Ready to be installed?
> >
> > + /* Reverse intervals exactly once. */
> > + if (v == 1)
> > + limits[index]->reverse ();
> >
> > why not do this at option processing time?
>
> Because we don't have there any dbgcnt API. Right now one can use multiple
> -fdbg-cnt options on a command line and that's why I'm doing that lazily.
But there's dbg_cnt_process_opt, that one can clearly insert intervals
in appropriately sorted way.
> > I think sorted insertion
> > is reasonable here (I don't see you sorting them at all?).
>
> I do expect a sorted intervals by user. If it's not provided, an error message
> is directly emitted.
I see, I find it overzealous esp. for multiple options like
-fdbg-cnt=foo:5-7 -fdbg-cnt=foo:2-3?
But yeah, it simplifies things. Still you could insert at the head
and avoid reversal.
Btw, there's vec::bsearch which might be convenient for sorted
insertion (even though
that doesn't return an index but a pointer to the element).
> > -static unsigned int limit_low[debug_counter_number_of_counters];
> > +static auto_vec<limit_tuple>
> > *limits[debug_counter_number_of_counters] = {NULL};
> >
> > I think you can avoid one indirection here (vec<> is just one pointer) by
> > doing
> >
> > static vec<limit_tuple> limits[debug_counter_number_of_counters] = { vNULL
> > };
> >
> > ? Verify you get no runtime ctor, not sure if that's correct C++ for
> > initializing
> > all elements by vNULL, not just the first...
> >
> > Alternatively I'd lazily allocate the whole vector rather than each
> > individual
> > vector (at option processing time again), so
> >
> > static vec<limit_tuple> (*limits)[];
>
> I fully agree with that we should reduce one level of indirection.
> Lemme take a look..
>
> Thanks,
> Martin
>
> >
> > Thanks,
> > Richard.
> >
> >> Thanks,
> >> Martin
> >>
> >> gcc/ChangeLog:
> >>
> >> 2019-11-08 Martin Liska <mli...@suse.cz>
> >>
> >> * common.opt: Document change of -fdbg-cnt option.
> >> * dbgcnt.c (DEBUG_COUNTER): Remove.
> >> (dbg_cnt_is_enabled): Remove.
> >> (dbg_cnt): Work with new intervals.
> >> (dbg_cnt_set_limit_by_index): Set to new
> >> list of intervals.
> >> (dbg_cnt_set_limit_by_name): Likewise.
> >> (dbg_cnt_process_single_pair): Process new format.
> >> (dbg_cnt_process_opt): Likewise.
> >> (dbg_cnt_list_all_counters): Likewise.
> >> * doc/invoke.texi: Document change of -fdbg-cnt option.
> >>
> >> gcc/testsuite/ChangeLog:
> >>
> >> 2019-11-08 Martin Liska <mli...@suse.cz>
> >>
> >> * gcc.dg/ipa/ipa-icf-39.c: Update -fdbg-cnt to the new format.
> >> * gcc.dg/pr68766.c: Likewise.
> >> ---
> >> gcc/common.opt | 2 +-
> >> gcc/dbgcnt.c | 157 +++++++++++++++-----------
> >> gcc/doc/invoke.texi | 15 ++-
> >> gcc/testsuite/gcc.dg/ipa/ipa-icf-39.c | 3 +-
> >> gcc/testsuite/gcc.dg/pr68766.c | 1 -
> >> 5 files changed, 103 insertions(+), 75 deletions(-)
> >>
> >>
>
