[3/4] gcov: Add gen_counter_update()

Message ID 20231114220825.22074-4-sebastian.huber@embedded-brains.de
State Accepted
Headers
Series gcov: Improve -fprofile-update=atomic |

Checks

Context Check Description
snail/gcc-patch-check success Github commit url

Commit Message

Sebastian Huber Nov. 14, 2023, 10:08 p.m. UTC
  Move the counter update to the new gen_counter_update() helper function.  Use
it in gimple_gen_edge_profiler() and gimple_gen_time_profiler().  The resulting
gimple instructions should be identical with the exception of the removed
unshare_expr() call.  The unshare_expr() call was used in
gimple_gen_edge_profiler().

gcc/ChangeLog:

	* tree-profile.cc (gen_assign_counter_update): New.
	(gen_counter_update): Likewise.
 	(gimple_gen_edge_profiler): Use gen_counter_update().
	(gimple_gen_time_profiler): Likewise.
---
 gcc/tree-profile.cc | 133 +++++++++++++++++++++-----------------------
 1 file changed, 62 insertions(+), 71 deletions(-)
  

Comments

Dimitar Dimitrov Nov. 19, 2023, 9 a.m. UTC | #1
On Tue, Nov 14, 2023 at 11:08:24PM +0100, Sebastian Huber wrote:
> Move the counter update to the new gen_counter_update() helper function.  Use
> it in gimple_gen_edge_profiler() and gimple_gen_time_profiler().  The resulting
> gimple instructions should be identical with the exception of the removed
> unshare_expr() call.  The unshare_expr() call was used in
> gimple_gen_edge_profiler().
> 
> gcc/ChangeLog:
> 
> 	* tree-profile.cc (gen_assign_counter_update): New.
> 	(gen_counter_update): Likewise.
>  	(gimple_gen_edge_profiler): Use gen_counter_update().
> 	(gimple_gen_time_profiler): Likewise.
> ---
>  gcc/tree-profile.cc | 133 +++++++++++++++++++++-----------------------
>  1 file changed, 62 insertions(+), 71 deletions(-)
> 

Hi Sebastian,

This patch caused a bunch of test failures on arm-none-eabi and
pru-unknown-elf targets.  One example:

/home/dinux/projects/pru/testbot-workspace/gcc/gcc/testsuite/gcc.dg/no_profile_instrument_function-attr-1.c: In function 'main':
/home/dinux/projects/pru/testbot-workspace/gcc/gcc/testsuite/gcc.dg/no_profile_instrument_function-attr-1.c:19:1: error: incorrect sharing of tree nodes
__gcov0.main[0]
# .MEM_12 = VDEF <.MEM_9>
__gcov0.main[0] = PROF_edge_counter_4;
during IPA pass: profile
/home/dinux/projects/pru/testbot-workspace/gcc/gcc/testsuite/gcc.dg/no_profile_instrument_function-attr-1.c:19:1: internal compiler error: verify_gimple failed
0xfd9c7d verify_gimple_in_cfg(function*, bool, bool)
        /home/dinux/projects/pru/testbot-workspace/gcc/gcc/tree-cfg.cc:5662
0xe586a4 execute_function_todo
        /home/dinux/projects/pru/testbot-workspace/gcc/gcc/passes.cc:2088
0xe58ba2 do_per_function
        /home/dinux/projects/pru/testbot-workspace/gcc/gcc/passes.cc:1694
0xe58ba2 do_per_function
        /home/dinux/projects/pru/testbot-workspace/gcc/gcc/passes.cc:1684
0xe58bfe execute_todo
        /home/dinux/projects/pru/testbot-workspace/gcc/gcc/passes.cc:2142
Please submit a full bug report, with preprocessed source (by using -freport-bug).
Please include the complete backtrace with any bug report.
See <https://gcc.gnu.org/bugs/> for instructions.
compiler exited with status 1
FAIL: gcc.dg/no_profile_instrument_function-attr-1.c (internal compiler error: verify_gimple failed)
FAIL: gcc.dg/no_profile_instrument_function-attr-1.c (test for excess errors)


I'm using the following script to build and test:
  https://github.com/dinuxbg/gnupru/blob/master/testing/manual-test-pru.sh

Regards,
Dimitar
  
Sebastian Huber Nov. 19, 2023, 2:43 p.m. UTC | #2
Hello Dimitar,

On 19.11.23 10:00, Dimitar Dimitrov wrote:
> On Tue, Nov 14, 2023 at 11:08:24PM +0100, Sebastian Huber wrote:
>> Move the counter update to the new gen_counter_update() helper function.  Use
>> it in gimple_gen_edge_profiler() and gimple_gen_time_profiler().  The resulting
>> gimple instructions should be identical with the exception of the removed
>> unshare_expr() call.  The unshare_expr() call was used in
>> gimple_gen_edge_profiler().
>>
>> gcc/ChangeLog:
>>
>> 	* tree-profile.cc (gen_assign_counter_update): New.
>> 	(gen_counter_update): Likewise.
>>   	(gimple_gen_edge_profiler): Use gen_counter_update().
>> 	(gimple_gen_time_profiler): Likewise.
>> ---
>>   gcc/tree-profile.cc | 133 +++++++++++++++++++++-----------------------
>>   1 file changed, 62 insertions(+), 71 deletions(-)
>>
> Hi Sebastian,
> 
> This patch caused a bunch of test failures on arm-none-eabi and
> pru-unknown-elf targets.

thanks for the report. I will have a look at this next week. I guess it 
has something to do with the removed unshare_expr() call. I don't really 
know what it does, but I will try to figure this out.
  

Patch

diff --git a/gcc/tree-profile.cc b/gcc/tree-profile.cc
index da300d5f9e8d..24805ff905c7 100644
--- a/gcc/tree-profile.cc
+++ b/gcc/tree-profile.cc
@@ -235,47 +235,78 @@  gimple_init_gcov_profiler (void)
     }
 }
 
-/* Output instructions as GIMPLE trees to increment the edge
-   execution count, and insert them on E.  We rely on
-   gsi_insert_on_edge to preserve the order.  */
+/* If RESULT is not null, then output instructions as GIMPLE trees to assign
+   the updated counter from CALL of FUNC to RESULT.  Insert the CALL and the
+   optional assignment instructions to GSI.  Use NAME for temporary values.  */
 
-void
-gimple_gen_edge_profiler (int edgeno, edge e)
+static inline void
+gen_assign_counter_update (gimple_stmt_iterator *gsi, gcall *call, tree func,
+			   tree result, const char *name)
 {
-  tree one;
+  if (result)
+    {
+      tree result_type = TREE_TYPE (TREE_TYPE (func));
+      tree tmp = make_temp_ssa_name (result_type, NULL, name);
+      gimple_set_lhs (call, tmp);
+      gsi_insert_after (gsi, call, GSI_NEW_STMT);
+      gassign *assign = gimple_build_assign (result, tmp);
+      gsi_insert_after (gsi, assign, GSI_NEW_STMT);
+    }
+  else
+    gsi_insert_after (gsi, call, GSI_NEW_STMT);
+}
 
-  one = build_int_cst (gcov_type_node, 1);
+/* Output instructions as GIMPLE trees to increment the COUNTER.  If RESULT is
+   not null, then assign the updated counter value to RESULT.  Insert the
+   instructions to GSI.  Use NAME for temporary values.  */
+
+static inline void
+gen_counter_update (gimple_stmt_iterator *gsi, tree counter, tree result,
+		    const char *name)
+{
+  tree type = gcov_type_node;
+  tree addr = build_fold_addr_expr (counter);
+  tree one = build_int_cst (type, 1);
+  tree relaxed = build_int_cst (integer_type_node, MEMMODEL_RELAXED);
 
   if (flag_profile_update == PROFILE_UPDATE_ATOMIC)
     {
       /* __atomic_fetch_add (&counter, 1, MEMMODEL_RELAXED); */
-      tree addr = tree_coverage_counter_addr (GCOV_COUNTER_ARCS, edgeno);
-      tree f = builtin_decl_explicit (TYPE_PRECISION (gcov_type_node) > 32
-				      ? BUILT_IN_ATOMIC_FETCH_ADD_8:
-				      BUILT_IN_ATOMIC_FETCH_ADD_4);
-      gcall *stmt = gimple_build_call (f, 3, addr, one,
-				       build_int_cst (integer_type_node,
-						      MEMMODEL_RELAXED));
-      gsi_insert_on_edge (e, stmt);
+      tree f = builtin_decl_explicit (TYPE_PRECISION (type) > 32
+				      ? BUILT_IN_ATOMIC_ADD_FETCH_8:
+				      BUILT_IN_ATOMIC_ADD_FETCH_4);
+      gcall *call = gimple_build_call (f, 3, addr, one, relaxed);
+      gen_assign_counter_update (gsi, call, f, result, name);
     }
   else
     {
-      tree ref = tree_coverage_counter_ref (GCOV_COUNTER_ARCS, edgeno);
-      tree gcov_type_tmp_var = make_temp_ssa_name (gcov_type_node,
-						   NULL, "PROF_edge_counter");
-      gassign *stmt1 = gimple_build_assign (gcov_type_tmp_var, ref);
-      gcov_type_tmp_var = make_temp_ssa_name (gcov_type_node,
-					      NULL, "PROF_edge_counter");
-      gassign *stmt2 = gimple_build_assign (gcov_type_tmp_var, PLUS_EXPR,
-					    gimple_assign_lhs (stmt1), one);
-      gassign *stmt3 = gimple_build_assign (unshare_expr (ref),
-					    gimple_assign_lhs (stmt2));
-      gsi_insert_on_edge (e, stmt1);
-      gsi_insert_on_edge (e, stmt2);
-      gsi_insert_on_edge (e, stmt3);
+      tree tmp1 = make_temp_ssa_name (type, NULL, name);
+      gassign *assign1 = gimple_build_assign (tmp1, counter);
+      gsi_insert_after (gsi, assign1, GSI_NEW_STMT);
+      tree tmp2 = make_temp_ssa_name (type, NULL, name);
+      gassign *assign2 = gimple_build_assign (tmp2, PLUS_EXPR, tmp1, one);
+      gsi_insert_after (gsi, assign2, GSI_NEW_STMT);
+      gassign *assign3 = gimple_build_assign (counter, tmp2);
+      gsi_insert_after (gsi, assign3, GSI_NEW_STMT);
+      if (result)
+	{
+	  gassign *assign4 = gimple_build_assign (result, tmp2);
+	  gsi_insert_after (gsi, assign4, GSI_NEW_STMT);
+	}
     }
 }
 
+/* Output instructions as GIMPLE trees to increment the edge
+   execution count, and insert them on E.  */
+
+void
+gimple_gen_edge_profiler (int edgeno, edge e)
+{
+  gimple_stmt_iterator gsi = gsi_last (PENDING_STMT (e));
+  tree counter = tree_coverage_counter_ref (GCOV_COUNTER_ARCS, edgeno);
+  gen_counter_update (&gsi, counter, NULL_TREE, "PROF_edge_counter");
+}
+
 /* Emits code to get VALUE to instrument at GSI, and returns the
    variable containing the value.  */
 
@@ -507,56 +538,16 @@  gimple_gen_time_profiler (unsigned tag)
   tree original_ref = tree_coverage_counter_ref (tag, 0);
   tree ref = force_gimple_operand_gsi (&gsi, original_ref, true, NULL_TREE,
 				       true, GSI_SAME_STMT);
-  tree one = build_int_cst (type, 1);
 
   /* Emit: if (counters[0] != 0).  */
   gcond *cond = gimple_build_cond (EQ_EXPR, ref, build_int_cst (type, 0),
 				   NULL, NULL);
   gsi_insert_before (&gsi, cond, GSI_NEW_STMT);
 
-  gsi = gsi_start_bb (update_bb);
-
   /* Emit: counters[0] = ++__gcov_time_profiler_counter.  */
-  if (flag_profile_update == PROFILE_UPDATE_ATOMIC)
-    {
-      tree ptr = make_temp_ssa_name (build_pointer_type (type), NULL,
-				     "PROF_time_profiler_counter_ptr");
-      tree addr = build1 (ADDR_EXPR, TREE_TYPE (ptr),
-			  tree_time_profiler_counter);
-      gassign *assign = gimple_build_assign (ptr, NOP_EXPR, addr);
-      gsi_insert_before (&gsi, assign, GSI_NEW_STMT);
-      tree f = builtin_decl_explicit (TYPE_PRECISION (gcov_type_node) > 32
-				      ? BUILT_IN_ATOMIC_ADD_FETCH_8:
-				      BUILT_IN_ATOMIC_ADD_FETCH_4);
-      gcall *stmt = gimple_build_call (f, 3, ptr, one,
-				       build_int_cst (integer_type_node,
-						      MEMMODEL_RELAXED));
-      tree result_type = TREE_TYPE (TREE_TYPE (f));
-      tree tmp = make_temp_ssa_name (result_type, NULL, "PROF_time_profile");
-      gimple_set_lhs (stmt, tmp);
-      gsi_insert_after (&gsi, stmt, GSI_NEW_STMT);
-      tmp = make_temp_ssa_name (type, NULL, "PROF_time_profile");
-      assign = gimple_build_assign (tmp, NOP_EXPR,
-				    gimple_call_lhs (stmt));
-      gsi_insert_after (&gsi, assign, GSI_NEW_STMT);
-      assign = gimple_build_assign (original_ref, tmp);
-      gsi_insert_after (&gsi, assign, GSI_NEW_STMT);
-    }
-  else
-    {
-      tree tmp = make_temp_ssa_name (type, NULL, "PROF_time_profile");
-      gassign *assign = gimple_build_assign (tmp, tree_time_profiler_counter);
-      gsi_insert_before (&gsi, assign, GSI_NEW_STMT);
-
-      tmp = make_temp_ssa_name (type, NULL, "PROF_time_profile");
-      assign = gimple_build_assign (tmp, PLUS_EXPR, gimple_assign_lhs (assign),
-				    one);
-      gsi_insert_after (&gsi, assign, GSI_NEW_STMT);
-      assign = gimple_build_assign (original_ref, tmp);
-      gsi_insert_after (&gsi, assign, GSI_NEW_STMT);
-      assign = gimple_build_assign (tree_time_profiler_counter, tmp);
-      gsi_insert_after (&gsi, assign, GSI_NEW_STMT);
-    }
+  gsi = gsi_start_bb (update_bb);
+  gen_counter_update (&gsi, tree_time_profiler_counter, original_ref,
+		      "PROF_time_profile");
 }
 
 /* Output instructions as GIMPLE trees to increment the average histogram