diff mbox

[Google] Fix discriminator assignment for stmts

Message ID CAO2gOZX2qA+Yy10sQf3NV4BF1K_tH02YS6AAbyup8jxEef6Lxw@mail.gmail.com
State New
Headers show

Commit Message

Dehao Chen July 3, 2013, 12:24 a.m. UTC 
This patch fixes the discriminator assignments for stmts. It used to
apply a signle locus for everything, which is not right (will
invalidate locus for macro expansion).

Bootstrapped and passed regression test.

OK for google-4_8?

Thanks,
Dehao

Comments

Cary Coutant July 3, 2013, 6:25 p.m. UTC | #1 
> -  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
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.)

-cary
diff mbox

Patch

Index: gcc/input.c
===================================================================
--- gcc/input.c (revision 200625)
+++ gcc/input.c (working copy)
@@ -308,7 +308,7 @@  location_with_discriminator (location_t locus, int
 {
   tree block = LOCATION_BLOCK (locus);
   location_t ret;
-  locus = LOCATION_LOCUS (locus);
+  locus = map_discriminator_location (locus);

   if (locus == UNKNOWN_LOCATION)
     return block ? COMBINE_LOCATION_DATA (line_table, locus, block)
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));
     }
 }