Patchwork [Google] Fix discriminator assignment for stmts

login
register
mail settings
Submitter Dehao Chen
Date July 3, 2013, 8 p.m.
Message ID <CAO2gOZWMBUOFQq_SCi1pf4mg5Obfci6Q1uKS6=DEe8bctDazrQ@mail.gmail.com>
Download mbox | patch
Permalink /patch/256733/
State New
Headers show

Comments

Dehao Chen - July 3, 2013, 8 p.m.
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.

Thanks,
Dehao

>
> -cary

Patch

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