[PATCH] Enhance syntax of -fdbg-cnt.

classic Classic list List threaded Threaded
17 messages Options
Reply | Threaded
Open this post in threaded view
|

[PATCH] Enhance syntax of -fdbg-cnt.

Martin Liška-2
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?
Thanks,
Martin

gcc/ChangeLog:

2019-11-08  Martin Liska  <[hidden email]>

        * 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  <[hidden email]>

        * 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(-)



0001-Enhance-syntax-of-fdbg-cnt.patch (9K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

[PATCH] Sort debug counter names.

Martin Liška-2
Hi.

The small patch is about sorting of debug counter
values.

Ready to be installed?
Thanks,
Martin

0001-Sort-debug-counter-names.patch (2K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Enhance syntax of -fdbg-cnt.

Richard Biener-2
In reply to this post by Martin Liška-2
On Mon, Nov 11, 2019 at 9:17 AM Martin Liška <[hidden email]> 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?  I think sorted insertion
is reasonable here (I don't see you sorting them at all?).

-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)[];

Thanks,
Richard.

> Thanks,
> Martin
>
> gcc/ChangeLog:
>
> 2019-11-08  Martin Liska  <[hidden email]>
>
>         * 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  <[hidden email]>
>
>         * 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(-)
>
>
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Sort debug counter names.

Richard Biener-2
In reply to this post by Martin Liška-2
On Mon, Nov 11, 2019 at 9:23 AM Martin Liška <[hidden email]> wrote:
>
> Hi.
>
> The small patch is about sorting of debug counter
> values.
>
> Ready to be installed?

OK

> Thanks,
> Martin
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Enhance syntax of -fdbg-cnt.

Martin Liška-2
In reply to this post by Richard Biener-2
On 11/11/19 3:19 PM, Richard Biener wrote:

> On Mon, Nov 11, 2019 at 9:17 AM Martin Liška <[hidden email]> 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.

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

>
> -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  <[hidden email]>
>>
>>          * 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  <[hidden email]>
>>
>>          * 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(-)
>>
>>

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Enhance syntax of -fdbg-cnt.

Richard Biener-2
On Mon, Nov 11, 2019 at 3:33 PM Martin Liška <[hidden email]> wrote:

>
> On 11/11/19 3:19 PM, Richard Biener wrote:
> > On Mon, Nov 11, 2019 at 9:17 AM Martin Liška <[hidden email]> 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  <[hidden email]>
> >>
> >>          * 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  <[hidden email]>
> >>
> >>          * 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(-)
> >>
> >>
>
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Enhance syntax of -fdbg-cnt.

Martin Liška-2
In reply to this post by Richard Biener-2
On 11/11/19 3:19 PM, Richard Biener wrote:
> -static unsigned int limit_low[debug_counter_number_of_counters];
> +static auto_vec<limit_tuple>
> *limits[debug_counter_number_of_counters] = {NULL};

Hm, apparently it's not working. I see a stack corruption when calling
dbgcnt. I also explicitly called .create (2) for all vectors, but the
ICE still happens.

Anyway, I would probably go with the original patch. It's handy for me
to also have limits[index] == NULL to indicate a counter that is not set.
That's a different state from limits[index].is_empty() which means all
previous intervals were popped and we should return false.

Martin
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Enhance syntax of -fdbg-cnt.

Martin Liška-2
In reply to this post by Richard Biener-2
On 11/11/19 3:50 PM, Richard Biener wrote:

> On Mon, Nov 11, 2019 at 3:33 PM Martin Liška <[hidden email]> wrote:
>>
>> On 11/11/19 3:19 PM, Richard Biener wrote:
>>> On Mon, Nov 11, 2019 at 9:17 AM Martin Liška <[hidden email]> 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.
Yes, I'm doing that right now.

>
>>> 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.
I decided to sort the intervals for during each insertion.
It should not affect performance as one does not insert hundreds of intervals.
Doing that I can do following error detection:

$ g++ -O2 tramp3d-v4.ii -fdbg-cnt=match:2-10 -fdbg-cnt=match:200-300 -fdbg-cnt=match:6-7 -fdbg-cnt-list
cc1plus: error: Interval overlap of ‘-fdbg-cnt=match’: [2, 10] and [6, 7]

I'm going to test the patch.
Martin

> 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  <[hidden email]>
>>>>
>>>>           * 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  <[hidden email]>
>>>>
>>>>           * 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(-)
>>>>
>>>>
>>


0001-Enhance-syntax-of-fdbg-cnt.patch (11K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Enhance syntax of -fdbg-cnt.

Martin Liška-2
On 11/11/19 4:25 PM, Martin Liška wrote:
>
> I'm going to test the patch.
> Martin

There's one more version where I use more references to mitigate
indirection of 'counter' vectors.

Martin

0001-Enhance-syntax-of-fdbg-cnt.patch (11K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Sort debug counter names.

Martin Liška-2
In reply to this post by Richard Biener-2
On 11/11/19 3:20 PM, Richard Biener wrote:

> On Mon, Nov 11, 2019 at 9:23 AM Martin Liška <[hidden email]> wrote:
>>
>> Hi.
>>
>> The small patch is about sorting of debug counter
>> values.
>>
>> Ready to be installed?
>
> OK
>
>> Thanks,
>> Martin
Hi.

There's a small follow up where I enforce the sorting via a selftest.

Ready for trunk?
Thanks,
Martin

0001-Come-up-with-selftests-for-dbgcnt.patch (2K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Enhance syntax of -fdbg-cnt.

Richard Biener-2
In reply to this post by Martin Liška-2
On Mon, Nov 11, 2019 at 3:56 PM Martin Liška <[hidden email]> wrote:
>
> On 11/11/19 3:19 PM, Richard Biener wrote:
> > -static unsigned int limit_low[debug_counter_number_of_counters];
> > +static auto_vec<limit_tuple>
> > *limits[debug_counter_number_of_counters] = {NULL};
>
> Hm, apparently it's not working. I see a stack corruption when calling
> dbgcnt. I also explicitly called .create (2) for all vectors, but the
> ICE still happens.

You did use vec<> instead of auto_vec<> , did you?  Using auto_vec<>
here IMHO is bogus anyways.

> Anyway, I would probably go with the original patch. It's handy for me
> to also have limits[index] == NULL to indicate a counter that is not set.

But there's vec.exists_p () which just checks whether the m_vec member
is NULL.

Since file-scope statics are zero-initialized by default even

static vec<limit_tuple> limits[debug_counter_number_of_counters];

should work.

> That's a different state from limits[index].is_empty() which means all
> previous intervals were popped and we should return false.

Yes, is_empty() vs. exists_p().

> Martin
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Sort debug counter names.

Richard Biener-2
In reply to this post by Martin Liška-2
On Tue, Nov 12, 2019 at 8:50 AM Martin Liška <[hidden email]> wrote:

>
> On 11/11/19 3:20 PM, Richard Biener wrote:
> > On Mon, Nov 11, 2019 at 9:23 AM Martin Liška <[hidden email]> wrote:
> >>
> >> Hi.
> >>
> >> The small patch is about sorting of debug counter
> >> values.
> >>
> >> Ready to be installed?
> >
> > OK
> >
> >> Thanks,
> >> Martin
>
> Hi.
>
> There's a small follow up where I enforce the sorting via a selftest.

Well, it's either a bit over-engineering unless you also make use of this and
optimize

  int i;
  for (i = debug_counter_number_of_counters - 1; i >= 0; i--)
    if (strcmp (map[i].name, name) == 0)
      break;

no?

> Ready for trunk?
> Thanks,
> Martin
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Enhance syntax of -fdbg-cnt.

Martin Liška-2
In reply to this post by Richard Biener-2
On 11/12/19 9:35 AM, Richard Biener wrote:

> On Mon, Nov 11, 2019 at 3:56 PM Martin Liška <[hidden email]> wrote:
>>
>> On 11/11/19 3:19 PM, Richard Biener wrote:
>>> -static unsigned int limit_low[debug_counter_number_of_counters];
>>> +static auto_vec<limit_tuple>
>>> *limits[debug_counter_number_of_counters] = {NULL};
>>
>> Hm, apparently it's not working. I see a stack corruption when calling
>> dbgcnt. I also explicitly called .create (2) for all vectors, but the
>> ICE still happens.
>
> You did use vec<> instead of auto_vec<> , did you?  Using auto_vec<>
> here IMHO is bogus anyways.
>
>> Anyway, I would probably go with the original patch. It's handy for me
>> to also have limits[index] == NULL to indicate a counter that is not set.
>
> But there's vec.exists_p () which just checks whether the m_vec member
> is NULL.
>
> Since file-scope statics are zero-initialized by default even
>
> static vec<limit_tuple> limits[debug_counter_number_of_counters];
>
> should work.
Works for me ;)

There's one another version that I'm testing right now.

Martin

>
>> That's a different state from limits[index].is_empty() which means all
>> previous intervals were popped and we should return false.
>
> Yes, is_empty() vs. exists_p().
>
>> Martin


0001-Enhance-syntax-of-fdbg-cnt.patch (11K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Sort debug counter names.

Martin Liška-2
In reply to this post by Richard Biener-2
On 11/12/19 9:39 AM, Richard Biener wrote:

> On Tue, Nov 12, 2019 at 8:50 AM Martin Liška <[hidden email]> wrote:
>>
>> On 11/11/19 3:20 PM, Richard Biener wrote:
>>> On Mon, Nov 11, 2019 at 9:23 AM Martin Liška <[hidden email]> wrote:
>>>>
>>>> Hi.
>>>>
>>>> The small patch is about sorting of debug counter
>>>> values.
>>>>
>>>> Ready to be installed?
>>>
>>> OK
>>>
>>>> Thanks,
>>>> Martin
>>
>> Hi.
>>
>> There's a small follow up where I enforce the sorting via a selftest.
>
> Well, it's either a bit over-engineering unless you also make use of this and
> optimize

My motivation was to preserve sorted list in -dbg-cnt-list.

>
>    int i;
>    for (i = debug_counter_number_of_counters - 1; i >= 0; i--)
>      if (strcmp (map[i].name, name) == 0)
>        break;
>
> no?

Heh, that can be rewritten to vec::bsearch, but would need to convert 'map'
to a 'vec' type. Or ideally to use a hash_map, but it will probably not pay back.

I don't insist on the patch :)

Martin

>
>> Ready for trunk?
>> Thanks,
>> Martin

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Enhance syntax of -fdbg-cnt.

Martin Liška-2
In reply to this post by Martin Liška-2
On 11/12/19 10:00 AM, Martin Liška wrote:
> There's one another version that I'm testing right now.

It survives regression tests and bootstrap.

Richi, may I install it the patch?

Thanks,
Martin
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Enhance syntax of -fdbg-cnt.

Richard Biener-2
On Wed, Nov 13, 2019 at 10:15 AM Martin Liška <[hidden email]> wrote:
>
> On 11/12/19 10:00 AM, Martin Liška wrote:
> > There's one another version that I'm testing right now.
>
> It survives regression tests and bootstrap.
>
> Richi, may I install it the patch?

Yes.

Thanks,
Richard.

>
> Thanks,
> Martin
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Sort debug counter names.

Richard Biener-2
In reply to this post by Martin Liška-2
On Tue, Nov 12, 2019 at 10:01 AM Martin Liška <[hidden email]> wrote:

>
> On 11/12/19 9:39 AM, Richard Biener wrote:
> > On Tue, Nov 12, 2019 at 8:50 AM Martin Liška <[hidden email]> wrote:
> >>
> >> On 11/11/19 3:20 PM, Richard Biener wrote:
> >>> On Mon, Nov 11, 2019 at 9:23 AM Martin Liška <[hidden email]> wrote:
> >>>>
> >>>> Hi.
> >>>>
> >>>> The small patch is about sorting of debug counter
> >>>> values.
> >>>>
> >>>> Ready to be installed?
> >>>
> >>> OK
> >>>
> >>>> Thanks,
> >>>> Martin
> >>
> >> Hi.
> >>
> >> There's a small follow up where I enforce the sorting via a selftest.
> >
> > Well, it's either a bit over-engineering unless you also make use of this and
> > optimize
>
> My motivation was to preserve sorted list in -dbg-cnt-list.

Ah, OK, that makes sense.  OTOH the function printing could just
sort it itself.

> >
> >    int i;
> >    for (i = debug_counter_number_of_counters - 1; i >= 0; i--)
> >      if (strcmp (map[i].name, name) == 0)
> >        break;
> >
> > no?
>
> Heh, that can be rewritten to vec::bsearch, but would need to convert 'map'
> to a 'vec' type. Or ideally to use a hash_map, but it will probably not pay back.
>
> I don't insist on the patch :)

Well, the above motivation is a good one so I guess ... OK.

Richard.

> Martin
>
> >
> >> Ready for trunk?
> >> Thanks,
> >> Martin
>