Please ignore the original patch, which is wrong. New patch updated, passed regression test.
Dehao On Wed, Jul 3, 2013 at 1:00 PM, Dehao Chen <de...@google.com> wrote: > On Wed, Jul 3, 2013 at 11:25 AM, Cary Coutant <ccout...@google.com> wrote: >> >> > - locus = location_with_discriminator ( >> > - locus, next_discriminator_for_locus (locus)); >> > + discriminator = next_discriminator_for_locus (locus); >> > >> > - if (block != NULL) >> > - locus = COMBINE_LOCATION_DATA (line_table, locus, block); >> > - >> > for (gsi = gsi_start_bb (bb); !gsi_end_p (gsi); gsi_next (&gsi)) >> > { >> > gimple stmt = gsi_stmt (gsi); >> > - if (same_line_p (locus, gimple_location (stmt))) >> > - gimple_set_location (stmt, locus); >> > + location_t stmt_locus = gimple_location (stmt); >> > + if (same_line_p (locus, stmt_locus)) >> > + gimple_set_location ( >> > + stmt, location_with_discriminator (stmt_locus, discriminator)); >> > } >> > } >> >> Sorry, this may be right, but I'm not yet convinced. Check my reasoning here: >> >> So we're back to same_line_p returns true if the two loci have the >> same spelling point, right? We're looping through the gimple stmts in > > We actually change to expand_location instead, patch updated. > >> the bb, looking for stmts that have the same spelling point, then >> assigning a new locus using that discriminator. Since the >> discriminator is associated with the spelling point, this won't be >> using one discriminator for different lines, but you may need to >> create a new locus in case the spelling point is the same but the >> expansion point is different. But location_with_discriminator is going >> to allocate a new locus unconditionally, so this will create a new >> locus for each stmt in the bb, even if they have the same locus to >> begin with. On large programs, I be afraid that this may exhaust the >> available supply of location_t values. >> >> How about saving the original locus and checking for straightforward >> equality first? If the stmt's locus is equal to the starting one, then >> just set it to the new locus; otherwise, check for same_line_p and >> create a new locus if that returns true. Something like this >> (untested): >> >> assign_discriminator (location_t locus, basic_block bb) >> { >> gimple_stmt_iterator gsi; >> int discriminator; >> location_t new_locus; >> >> locus = map_discriminator_location (locus); >> >> if (locus == UNKNOWN_LOCATION) >> return; >> >> discriminator = next_discriminator_for_locus (locus); >> new_locus = location_with_discriminator (locus, discriminator) >> >> for (gsi = gsi_start_bb (bb); !gsi_end_p (gsi); gsi_next (&gsi)) >> { >> gimple stmt = gsi_stmt (gsi); >> location_t stmt_locus = gimple_location (stmt); >> >> if (locus == stmt_locus) >> gimple_set_location (stmt, new_locus); >> else if (same_line_p (locus, stmt_locus)) >> gimple_set_location ( >> stmt, location_with_discriminator (stmt_locus, discriminator)); >> } >> } >> >> This could still allocate lots of unnecessary new location_t values, >> though (and may never use new_locus). Maybe the right thing to do >> would be to have location_with_discriminator implement a rudimentary >> cache to avoid handing out a new location_t value when a >> recently-allocated one matches. (I don't think you even need a cache >> -- it can just look back at the last few elements in >> discriminator_location_locations and >> discriminator_location_discriminators.) > > You are right. I've updated the patch to do this. Testing on-going. > > Index: gcc/input.c > =================================================================== > --- gcc/input.c (revision 200643) > +++ gcc/input.c (working copy) > @@ -308,7 +308,8 @@ location_with_discriminator (location_t locus, int > { > tree block = LOCATION_BLOCK (locus); > location_t ret; > - locus = LOCATION_LOCUS (locus); > + int i; > + locus = map_discriminator_location (locus); > > if (locus == UNKNOWN_LOCATION) > return block ? COMBINE_LOCATION_DATA (line_table, locus, block) > @@ -320,14 +321,23 @@ location_with_discriminator (location_t locus, int > next_discriminator_location = min_discriminator_location; > } > > + /* Traverse the last few discriminator_locations to see if we can reuse > + the entry. */ > + for (i = next_discriminator_location - min_discriminator_location - 1; > + i >= 0 && LOCATION_LINE (discriminator_location_locations[i]) == > + LOCATION_LINE (locus) && > + discriminator_location_discriminators[i] == discriminator; i--) > + if (discriminator_location_locations[i] == locus) > + return block ? next_discriminator_location - i : > + COMBINE_LOCATION_DATA (line_table, next_discriminator_location - i, > + block); > + > discriminator_location_locations.safe_push(locus); > discriminator_location_discriminators.safe_push(discriminator); > > - if (block != NULL) > - ret = COMBINE_LOCATION_DATA (line_table, next_discriminator_location, > - block); > - else > - ret = next_discriminator_location; > + ret = block ? next_discriminator_location : > + COMBINE_LOCATION_DATA (line_table, next_discriminator_location, > + block); > next_discriminator_location++; > return ret; > } > Index: gcc/tree-cfg.c > =================================================================== > --- gcc/tree-cfg.c (revision 200643) > +++ gcc/tree-cfg.c (working copy) > @@ -732,8 +732,8 @@ same_line_p (location_t locus1, location_t locus2) > if (locus1 == locus2) > return true; > > - from = expand_location_to_spelling_point (locus1); > - to = expand_location_to_spelling_point (locus2); > + from = expand_location (locus1); > + to = expand_location (locus2); > > if (from.line != to.line) > return false; > @@ -751,24 +751,22 @@ static void > assign_discriminator (location_t locus, basic_block bb) > { > gimple_stmt_iterator gsi; > - tree block = LOCATION_BLOCK (locus); > + int discriminator; > > locus = map_discriminator_location (locus); > > if (locus == UNKNOWN_LOCATION) > return; > > - locus = location_with_discriminator ( > - locus, next_discriminator_for_locus (locus)); > + discriminator = next_discriminator_for_locus (locus); > > - if (block != NULL) > - locus = COMBINE_LOCATION_DATA (line_table, locus, block); > - > for (gsi = gsi_start_bb (bb); !gsi_end_p (gsi); gsi_next (&gsi)) > { > gimple stmt = gsi_stmt (gsi); > - if (same_line_p (locus, gimple_location (stmt))) > - gimple_set_location (stmt, locus); > + location_t stmt_locus = gimple_location (stmt); > + if (same_line_p (locus, stmt_locus)) > + gimple_set_location ( > + stmt, location_with_discriminator (stmt_locus, discriminator)); > } > } > > Thanks, > Dehao > >> >> -cary
Index: gcc/input.c =================================================================== --- gcc/input.c (revision 200625) +++ gcc/input.c (working copy) @@ -31,7 +31,7 @@ location_t input_location; struct line_maps *line_table; static vec<location_t> discriminator_location_locations; -static vec<location_t> discriminator_location_discriminators; +static vec<int> discriminator_location_discriminators; static location_t next_discriminator_location = UNKNOWN_LOCATION; static location_t min_discriminator_location = UNKNOWN_LOCATION; @@ -308,7 +308,8 @@ location_with_discriminator (location_t locus, int { tree block = LOCATION_BLOCK (locus); location_t ret; - locus = LOCATION_LOCUS (locus); + int i; + locus = map_discriminator_location (locus); if (locus == UNKNOWN_LOCATION) return block ? COMBINE_LOCATION_DATA (line_table, locus, block) @@ -320,14 +321,24 @@ location_with_discriminator (location_t locus, int next_discriminator_location = min_discriminator_location; } + /* Traverse the last few discriminator_locations to see if we can reuse + the entry. */ + for (i = next_discriminator_location - min_discriminator_location - 1; + i >= 0 && LOCATION_LINE (discriminator_location_locations[i]) == + LOCATION_LINE (locus) && + discriminator_location_discriminators[i] == discriminator; i--) + if (discriminator_location_locations[i] == locus) + return block ? COMBINE_LOCATION_DATA ( + line_table, min_discriminator_location + i, block) + : min_discriminator_location + i; + discriminator_location_locations.safe_push(locus); discriminator_location_discriminators.safe_push(discriminator); - if (block != NULL) - ret = COMBINE_LOCATION_DATA (line_table, next_discriminator_location, - block); - else - ret = next_discriminator_location; + ret = block ? COMBINE_LOCATION_DATA ( + line_table, next_discriminator_location, block) + : next_discriminator_location; + next_discriminator_location++; return ret; } @@ -362,5 +373,5 @@ get_discriminator_from_locus (location_t locus) locus = LOCATION_LOCUS (locus); if (! has_discriminator (locus)) return 0; - return (location_t) discriminator_location_discriminators[locus - min_discriminator_location]; + return discriminator_location_discriminators[locus - min_discriminator_location]; } Index: gcc/tree-cfg.c =================================================================== --- gcc/tree-cfg.c (revision 200625) +++ gcc/tree-cfg.c (working copy) @@ -751,24 +751,22 @@ static void assign_discriminator (location_t locus, basic_block bb) { gimple_stmt_iterator gsi; - tree block = LOCATION_BLOCK (locus); + int discriminator; locus = map_discriminator_location (locus); if (locus == UNKNOWN_LOCATION) return; - locus = location_with_discriminator ( - locus, next_discriminator_for_locus (locus)); + discriminator = next_discriminator_for_locus (locus); - if (block != NULL) - locus = COMBINE_LOCATION_DATA (line_table, locus, block); - for (gsi = gsi_start_bb (bb); !gsi_end_p (gsi); gsi_next (&gsi)) { gimple stmt = gsi_stmt (gsi); - if (same_line_p (locus, gimple_location (stmt))) - gimple_set_location (stmt, locus); + location_t stmt_locus = gimple_location (stmt); + if (same_line_p (locus, stmt_locus)) + gimple_set_location ( + stmt, location_with_discriminator (stmt_locus, discriminator)); } }