Patchwork [Google] Fix discriminator assignment for stmts

login
register
mail settings
Submitter Dehao Chen
Date July 3, 2013, 9:11 p.m.
Message ID <CAO2gOZUrm8yquD7nw1c1e90oxNVYeWEhCqFbyE3LLAtkZqHR0A@mail.gmail.com>
Download mbox | patch
Permalink /patch/256746/
State New
Headers show

Comments

Dehao Chen - July 3, 2013, 9:11 p.m.
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 <dehao@google.com> wrote:
> On Wed, Jul 3, 2013 at 11:25 AM, Cary Coutant <ccoutant@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
Cary Coutant - July 3, 2013, 9:52 p.m.
> Please ignore the original patch, which is wrong. New patch updated,
> passed regression test.

+  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;

As per GCC coding conventions, the long expressions need to be
parenthesized and indented under the parens, with operators at the
beginning of the line, like this:

+  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);

Same here:

+  ret = block ? COMBINE_LOCATION_DATA (
+       line_table, next_discriminator_location, block)
+       : next_discriminator_location;

Should be something like this:

+  ret = (block
+         ? COMBINE_LOCATION_DATA (line_table, next_discriminator_location,
+                                  block)
+         : next_discriminator_location);

(Although this last statement would probably read better as a regular
if statement.)

And here:

+      if (same_line_p (locus, stmt_locus))
+        gimple_set_location (
+           stmt, location_with_discriminator (stmt_locus, discriminator));

Should be something like this:

+      if (same_line_p (locus, stmt_locus))
+        gimple_set_location (stmt,
+                             location_with_discriminator (stmt_locus,
+                                                          discriminator));

Looks good with those changes. Thanks!

-cary

Patch

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));
     }
 }