| Message ID | CA+4CFy5qHHuXhWhFjTzQUmvDs50QiQCitjsbhTCO5V-cha6Qnw@mail.gmail.com |
|---|---|
| State | New |
| Headers | show |
Hi Cary, Is the new patch ok for google-4_9? Thanks, Wei. On Sun, Aug 24, 2014 at 8:53 PM, Wei Mi <wmi@google.com> wrote: > To avoid the unused new discriminator value, I added a map > "found_call_this_line" to track whether a call is the first call in a > source line seen when assigning discriminators. For the first call in > a source line, its discriminator is 0. For the following calls in the > same source line, a new discriminator will be used everytime. The new > patch is attached. Internal perf test and regression test are ok. Is > it ok for google-4_9? > > Thanks, > Wei. > > > > On Thu, Aug 7, 2014 at 2:10 PM, Wei Mi <wmi@google.com> wrote: >> Yes, that is intentional. It is to avoid assiging a discriminator for >> the first call in the group of calls with the same source lineno. >> Starting from the second call in the group, it will get a different >> discriminator with previous call in the same group. >> >> Thanks, >> Wei. >> >> On Thu, Aug 7, 2014 at 12:17 PM, Cary Coutant <ccoutant@google.com> wrote: >>>> static int >>>> -next_discriminator_for_locus (location_t locus) >>>> +increase_discriminator_for_locus (location_t locus, bool return_next) >>>> { >>>> struct locus_discrim_map item; >>>> struct locus_discrim_map **slot; >>>> @@ -934,8 +936,10 @@ next_discriminator_for_locus (location_t >>>> (*slot)->locus = locus; >>>> (*slot)->discriminator = 0; >>>> } >>>> + >>>> (*slot)->discriminator++; >>>> - return (*slot)->discriminator; >>>> + return return_next ? (*slot)->discriminator >>>> + : (*slot)->discriminator - 1; >>>> } >>> >>> Won't this have the effect of sometimes incrementing the next >>> available discriminator without actually using the new value? That is, >>> if you call it once with return_next == false, and then with >>> return_next == true. >>> >>> -cary
> To avoid the unused new discriminator value, I added a map > "found_call_this_line" to track whether a call is the first call in a > source line seen when assigning discriminators. For the first call in > a source line, its discriminator is 0. For the following calls in the > same source line, a new discriminator will be used everytime. The new > patch is attached. Internal perf test and regression test are ok. Is > it ok for google-4_9? This seems overly complex to me. I'd think all you need to do is add a bit to locus_discrim_map (stealing a bit from discriminator ought to be fine) that indicates whether the next call should increment the discriminator or not. Something like this: increase_discriminator_for_locus (location_t locus, bool return_next) { ... if (return_next || (*slot)->needs_increment) { (*slot)->discriminator++; (*slot)->needs_increment = false; } else (*slot)->needs_increment = true; return (*slot)->discriminator; } -cary
Thanks, that is ellegant. Will paste a new patch in this way soon. Wei. On Fri, Aug 29, 2014 at 10:11 AM, Cary Coutant <ccoutant@google.com> wrote: >> To avoid the unused new discriminator value, I added a map >> "found_call_this_line" to track whether a call is the first call in a >> source line seen when assigning discriminators. For the first call in >> a source line, its discriminator is 0. For the following calls in the >> same source line, a new discriminator will be used everytime. The new >> patch is attached. Internal perf test and regression test are ok. Is >> it ok for google-4_9? > > This seems overly complex to me. I'd think all you need to do is add a > bit to locus_discrim_map (stealing a bit from discriminator ought to > be fine) that indicates whether the next call should increment the > discriminator or not. Something like this: > > increase_discriminator_for_locus (location_t locus, bool return_next) > { > ... > if (return_next || (*slot)->needs_increment) > { > (*slot)->discriminator++; > (*slot)->needs_increment = false; > } > else > (*slot)->needs_increment = true; > return (*slot)->discriminator; > } > > -cary
Index: tree-cfg.c =================================================================== --- tree-cfg.c (revision 213402) +++ tree-cfg.c (working copy) @@ -992,7 +992,13 @@ static void assign_discriminators (void) { basic_block bb; + /* If there is a location saved in the hash_table, it means that we + already found a call in the source line before. For the calls which + are not the first call found in the same source line, we don't assign + new discriminator for it, so that .debug_line section will be smaller. */ + hash_table <locus_discrim_hasher> found_call_this_line; + found_call_this_line.create (13); FOR_EACH_BB_FN (bb, cfun) { edge e; @@ -1009,23 +1015,31 @@ assign_discriminators (void) for (gsi = gsi_start_bb (bb); !gsi_end_p (gsi); gsi_next (&gsi)) { gimple stmt = gsi_stmt (gsi); - if (curr_locus == UNKNOWN_LOCATION) - { - curr_locus = gimple_location (stmt); - } - else if (!same_line_p (curr_locus, gimple_location (stmt))) + if (gimple_code (stmt) == GIMPLE_CALL) { + struct locus_discrim_map item; + struct locus_discrim_map **slot; + curr_locus = gimple_location (stmt); - curr_discr = 0; - } - else if (curr_discr != 0) - { - gimple_set_location (stmt, location_with_discriminator ( - gimple_location (stmt), curr_discr)); + item.locus = curr_locus; + item.discriminator = 0; + slot = found_call_this_line.find_slot (&item, INSERT); + /* If the current call is not the first call seen in curr_locus, + assign the next discriminator to it, else keep its discriminator + unchanged. */ + if (*slot != HTAB_EMPTY_ENTRY) + { + curr_discr = next_discriminator_for_locus (curr_locus); + gimple_set_location (stmt, location_with_discriminator ( + gimple_location (stmt), curr_discr)); + } + else + { + *slot = XNEW (struct locus_discrim_map); + (*slot)->locus = curr_locus; + (*slot)->discriminator = 0; + } } - /* Allocate a new discriminator for CALL stmt. */ - if (gimple_code (stmt) == GIMPLE_CALL) - curr_discr = next_discriminator_for_locus (curr_locus); } if (locus == UNKNOWN_LOCATION) @@ -1047,6 +1061,7 @@ assign_discriminators (void) } } } + found_call_this_line.dispose (); } /* Create the edges for a GIMPLE_COND starting at block BB. */