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 <[email protected]> wrote:
> On Wed, Jul 3, 2013 at 11:25 AM, Cary Coutant <[email protected]> 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));
}
}
