diff mbox series

Use 'location_hash' for 'seen_locations' in 'gcc/profile.c:branch_prob' (was: [PATCH] Fix GCOV CFG related issues)

Message ID 87fss5xv08.fsf@euler.schwinge.homeip.net
State Committed
Headers
Series Use 'location_hash' for 'seen_locations' in 'gcc/profile.c:branch_prob' (was: [PATCH] Fix GCOV CFG related issues) |

Commit Message

Thomas Schwinge Nov. 9, 2021, 2:29 p.m. UTC 
  Hi!

On 2018-07-25T15:40:24+0200, Martin Liška <mliska@suse.cz> wrote:
> --- a/gcc/profile.c
> +++ b/gcc/profile.c
> @@ -1256,6 +1256,8 @@ branch_prob (void)
>        /* Initialize the output.  */
>        output_location (NULL, 0, NULL, NULL);
>
> +      hash_set<int_hash <location_t, 0, 2> > seen_locations;
> +
>        FOR_EACH_BB_FN (bb, cfun)
>       {
>         gimple_stmt_iterator gsi;

Given my recent commit 088199e5d0fc0d54f48af0783a2630a773bbb387
"Generalize 'gcc/input.h:struct location_hash'", OK to push the attached
"Use 'location_hash' for 'seen_locations' in 'gcc/profile.c:branch_prob'"?


Grüße
 Thomas


> @@ -1263,8 +1265,9 @@ branch_prob (void)
>
>         if (bb == ENTRY_BLOCK_PTR_FOR_FN (cfun)->next_bb)
>           {
> -           expanded_location curr_location =
> -             expand_location (DECL_SOURCE_LOCATION (current_function_decl));
> +           location_t loc = DECL_SOURCE_LOCATION (current_function_decl);
> +           seen_locations.add (loc);
> +           expanded_location curr_location = expand_location (loc);
>             output_location (curr_location.file, curr_location.line,
>                              &offset, bb);
>           }
> @@ -1272,17 +1275,25 @@ branch_prob (void)
>         for (gsi = gsi_start_bb (bb); !gsi_end_p (gsi); gsi_next (&gsi))
>           {
>             gimple *stmt = gsi_stmt (gsi);
> -           if (!RESERVED_LOCATION_P (gimple_location (stmt)))
> -             output_location (gimple_filename (stmt), gimple_lineno (stmt),
> -                              &offset, bb);
> +           location_t loc = gimple_location (stmt);
> +           if (!RESERVED_LOCATION_P (loc))
> +             {
> +               seen_locations.add (loc);
> +               output_location (gimple_filename (stmt), gimple_lineno (stmt),
> +                                &offset, bb);
> +             }
>           }
>
> -       /* Notice GOTO expressions eliminated while constructing the CFG.  */
> +       /* Notice GOTO expressions eliminated while constructing the CFG.
> +          It's hard to distinguish such expression, but goto_locus should
> +          not be any of already seen location.  */
> +       location_t loc;
>         if (single_succ_p (bb)
> -           && !RESERVED_LOCATION_P (single_succ_edge (bb)->goto_locus))
> +           && (loc = single_succ_edge (bb)->goto_locus)
> +           && !RESERVED_LOCATION_P (loc)
> +           && !seen_locations.contains (loc))
>           {
> -           expanded_location curr_location
> -             = expand_location (single_succ_edge (bb)->goto_locus);
> +           expanded_location curr_location = expand_location (loc);
>             output_location (curr_location.file, curr_location.line,
>                              &offset, bb);
>           }


-----------------
Siemens Electronic Design Automation GmbH; Anschrift: Arnulfstraße 201, 80634 München; Gesellschaft mit beschränkter Haftung; Geschäftsführer: Thomas Heurung, Frank Thürauf; Sitz der Gesellschaft: München; Registergericht München, HRB 106955

Comments

Martin Liška Nov. 9, 2021, 2:38 p.m. UTC | #1 
On 11/9/21 15:29, Thomas Schwinge wrote:
> Hi!
> 
> On 2018-07-25T15:40:24+0200, Martin Liška <mliska@suse.cz> wrote:
>> --- a/gcc/profile.c
>> +++ b/gcc/profile.c
>> @@ -1256,6 +1256,8 @@ branch_prob (void)
>>         /* Initialize the output.  */
>>         output_location (NULL, 0, NULL, NULL);
>>
>> +      hash_set<int_hash <location_t, 0, 2> > seen_locations;
>> +
>>         FOR_EACH_BB_FN (bb, cfun)
>>        {
>>          gimple_stmt_iterator gsi;
> 
> Given my recent commit 088199e5d0fc0d54f48af0783a2630a773bbb387
> "Generalize 'gcc/input.h:struct location_hash'", OK to push the attached
> "Use 'location_hash' for 'seen_locations' in 'gcc/profile.c:branch_prob'"?

Yes, thanks.

Martin

> 
> 
> Grüße
>   Thomas
> 
> 
>> @@ -1263,8 +1265,9 @@ branch_prob (void)
>>
>>          if (bb == ENTRY_BLOCK_PTR_FOR_FN (cfun)->next_bb)
>>            {
>> -           expanded_location curr_location =
>> -             expand_location (DECL_SOURCE_LOCATION (current_function_decl));
>> +           location_t loc = DECL_SOURCE_LOCATION (current_function_decl);
>> +           seen_locations.add (loc);
>> +           expanded_location curr_location = expand_location (loc);
>>              output_location (curr_location.file, curr_location.line,
>>                               &offset, bb);
>>            }
>> @@ -1272,17 +1275,25 @@ branch_prob (void)
>>          for (gsi = gsi_start_bb (bb); !gsi_end_p (gsi); gsi_next (&gsi))
>>            {
>>              gimple *stmt = gsi_stmt (gsi);
>> -           if (!RESERVED_LOCATION_P (gimple_location (stmt)))
>> -             output_location (gimple_filename (stmt), gimple_lineno (stmt),
>> -                              &offset, bb);
>> +           location_t loc = gimple_location (stmt);
>> +           if (!RESERVED_LOCATION_P (loc))
>> +             {
>> +               seen_locations.add (loc);
>> +               output_location (gimple_filename (stmt), gimple_lineno (stmt),
>> +                                &offset, bb);
>> +             }
>>            }
>>
>> -       /* Notice GOTO expressions eliminated while constructing the CFG.  */
>> +       /* Notice GOTO expressions eliminated while constructing the CFG.
>> +          It's hard to distinguish such expression, but goto_locus should
>> +          not be any of already seen location.  */
>> +       location_t loc;
>>          if (single_succ_p (bb)
>> -           && !RESERVED_LOCATION_P (single_succ_edge (bb)->goto_locus))
>> +           && (loc = single_succ_edge (bb)->goto_locus)
>> +           && !RESERVED_LOCATION_P (loc)
>> +           && !seen_locations.contains (loc))
>>            {
>> -           expanded_location curr_location
>> -             = expand_location (single_succ_edge (bb)->goto_locus);
>> +           expanded_location curr_location = expand_location (loc);
>>              output_location (curr_location.file, curr_location.line,
>>                               &offset, bb);
>>            }
> 
> 
> -----------------
> Siemens Electronic Design Automation GmbH; Anschrift: Arnulfstraße 201, 80634 München; Gesellschaft mit beschränkter Haftung; Geschäftsführer: Thomas Heurung, Frank Thürauf; Sitz der Gesellschaft: München; Registergericht München, HRB 106955
>
diff mbox series

Patch

From 5d78a424466f3fc89f430c8b8282ce5820dffbe3 Mon Sep 17 00:00:00 2001
From: Thomas Schwinge <thomas@codesourcery.com>
Date: Tue, 31 Aug 2021 23:34:23 +0200
Subject: [PATCH] Use 'location_hash' for 'seen_locations' in
 'gcc/profile.c:branch_prob'

Follow-up to commit 102fcf94e625a2016a65829c73a42bd6c2420376
"Fix GCOV CFG related issues": considering the current
'int_hash <location_t, 0, 2>', per 'libcpp/include/line-map.h':

      Actual     | Value                         | Meaning
      -----------+-------------------------------+-------------------------------
      0x00000000 | UNKNOWN_LOCATION (gcc/input.h)| Unknown/invalid location.
      -----------+-------------------------------+-------------------------------
      0x00000001 | BUILTINS_LOCATION             | The location for declarations
                 |   (gcc/input.h)               | in "<built-in>"
      -----------+-------------------------------+-------------------------------
      0x00000002 | RESERVED_LOCATION_COUNT       | The first location to be
                 | (also                         | handed out, and the
                 |  ordmap[0]->start_location)   | first line in ordmap 0

... this currently uses value '0' ('UNKNOWN_LOCATION') as spare values for
'Empty', and value '2' ('RESERVED_LOCATION_COUNT') as spare values for
'Deleted', which is questionable?

What actually does get put into 'seen_locations' is (mostly...)
restricted/gated by '!RESERVED_LOCATION_P' (which is true unless
'UNKNOWN_LOCATION' or 'BUILTINS_LOCATION'), thus we may simply use
'location_hash'.

	gcc/
	* profile.c (branch_prob): Use 'location_hash' for
	'seen_locations'.
---
 gcc/profile.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/gcc/profile.c b/gcc/profile.c
index c33c833167f..d07002d265e 100644
--- a/gcc/profile.c
+++ b/gcc/profile.c
@@ -1375,7 +1375,7 @@  branch_prob (bool thunk)
       /* Initialize the output.  */
       output_location (&streamed_locations, NULL, 0, NULL, NULL);
 
-      hash_set<int_hash <location_t, 0, 2> > seen_locations;
+      hash_set<location_hash> seen_locations;
 
       FOR_EACH_BB_FN (bb, cfun)
 	{
@@ -1385,6 +1385,7 @@  branch_prob (bool thunk)
 	  if (bb == ENTRY_BLOCK_PTR_FOR_FN (cfun)->next_bb)
 	    {
 	      location_t loc = DECL_SOURCE_LOCATION (current_function_decl);
+	      gcc_checking_assert (!RESERVED_LOCATION_P (loc));
 	      seen_locations.add (loc);
 	      expanded_location curr_location = expand_location (loc);
 	      output_location (&streamed_locations, curr_location.file,
-- 
2.33.0