Re: [Google] Fix discriminator assignment for stmts

[Cary Coutant <ccoutant at google dot com>]

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