[PATCH 1/2] gcov: Use unshare_expr() in gen_counter_update()

[PATCH 1/2] gcov: Use unshare_expr() in gen_counter_update()

[Sebastian Huber]

This fixes issues like this:

  gcc/testsuite/gcc.dg/no_profile_instrument_function-attr-1.c: In function 'main':
  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
  gcc/testsuite/gcc.dg/no_profile_instrument_function-attr-1.c:19:1: internal compiler error: verify_gimple failed

Unshare the counter expression in the second gimple_build_assign() in
gen_counter_update().  This is similar to the original
gimple_gen_edge_profiler() for "ref":

void
gimple_gen_edge_profiler (int edgeno, edge e)
{
  tree one;

  one = build_int_cst (gcov_type_node, 1);

  if (flag_profile_update == PROFILE_UPDATE_ATOMIC)
[...]
  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);
    }
}

However, the orignal gimple_gen_time_profiler() did not use unshare_expr() for
the counter expression (tree_time_profiler_counter):

void
gimple_gen_time_profiler (unsigned tag)
{
[...]

  /* Emit: counters[0] = ++__gcov_time_profiler_counter.  */
  if (flag_profile_update == PROFILE_UPDATE_ATOMIC)
[...]
  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);
    }
}

gcc/ChangeLog:

	* tree-profile.cc (gen_counter_update): Use unshare_expr() for the
	counter expression in the second gimple_build_assign().
---
 gcc/tree-profile.cc | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/gcc/tree-profile.cc b/gcc/tree-profile.cc
index 7d3cb1ef089..68db09f6189 100644
--- a/gcc/tree-profile.cc
+++ b/gcc/tree-profile.cc
@@ -354,7 +354,7 @@ gen_counter_update (gimple_stmt_iterator *gsi, tree counter, tree result,
       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);
+      gassign *assign3 = gimple_build_assign (unshare_expr (counter), tmp2);
       gsi_insert_after (gsi, assign3, GSI_NEW_STMT);
       if (result)
 	{
-- 
2.35.3

[PATCH 2/2] gcov: Fix integer types in gen_counter_update()

[Sebastian Huber]

This change fixes issues like this:

  gcc.dg/gomp/pr27573.c: In function ‘main._omp_fn.0’:
  gcc.dg/gomp/pr27573.c:19:1: error: non-trivial conversion in ‘ssa_name’
     19 | }
        | ^
  long int
  long unsigned int
  # .MEM_19 = VDEF <.MEM_18>
  __gcov7.main._omp_fn.0[0] = PROF_time_profile_12;
  during IPA pass: profile
  gcc.dg/gomp/pr27573.c:19:1: internal compiler error: verify_gimple failed

gcc/ChangeLog:

	PR middle-end/112634

	* tree-profile.cc (gen_assign_counter_update): Cast the unsigned result type of
	__atomic_add_fetch() to the signed counter type.
---
 gcc/tree-profile.cc | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/gcc/tree-profile.cc b/gcc/tree-profile.cc
index 68db09f6189..54938e1d165 100644
--- a/gcc/tree-profile.cc
+++ b/gcc/tree-profile.cc
@@ -284,7 +284,9 @@ gen_assign_counter_update (gimple_stmt_iterator *gsi, gcall *call, tree 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);
+      gassign *assign = gimple_build_assign (result,
+					     build_int_cst (TREE_TYPE (result),
+							    tmp));
       gsi_insert_after (gsi, assign, GSI_NEW_STMT);
     }
   else
-- 
2.35.3

Re: [PATCH 1/2] gcov: Use unshare_expr() in gen_counter_update()

[Richard Biener]

On Mon, Nov 20, 2023 at 3:34 PM Sebastian Huber
<sebastian.huber@embedded-brains.de> wrote:
>
> This fixes issues like this:
>
>   gcc/testsuite/gcc.dg/no_profile_instrument_function-attr-1.c: In function 'main':
>   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
>   gcc/testsuite/gcc.dg/no_profile_instrument_function-attr-1.c:19:1: internal compiler error: verify_gimple failed
>
> Unshare the counter expression in the second gimple_build_assign() in
> gen_counter_update().  This is similar to the original
> gimple_gen_edge_profiler() for "ref":
>
> void
> gimple_gen_edge_profiler (int edgeno, edge e)
> {
>   tree one;
>
>   one = build_int_cst (gcov_type_node, 1);
>
>   if (flag_profile_update == PROFILE_UPDATE_ATOMIC)
> [...]
>   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);
>     }
> }
>
> However, the orignal gimple_gen_time_profiler() did not use unshare_expr() for
> the counter expression (tree_time_profiler_counter):
>
> void
> gimple_gen_time_profiler (unsigned tag)
> {
> [...]
>
>   /* Emit: counters[0] = ++__gcov_time_profiler_counter.  */
>   if (flag_profile_update == PROFILE_UPDATE_ATOMIC)
> [...]
>   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);
>     }
> }
>
> gcc/ChangeLog:

OK.

>         * tree-profile.cc (gen_counter_update): Use unshare_expr() for the
>         counter expression in the second gimple_build_assign().
> ---
>  gcc/tree-profile.cc | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/gcc/tree-profile.cc b/gcc/tree-profile.cc
> index 7d3cb1ef089..68db09f6189 100644
> --- a/gcc/tree-profile.cc
> +++ b/gcc/tree-profile.cc
> @@ -354,7 +354,7 @@ gen_counter_update (gimple_stmt_iterator *gsi, tree counter, tree result,
>        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);
> +      gassign *assign3 = gimple_build_assign (unshare_expr (counter), tmp2);
>        gsi_insert_after (gsi, assign3, GSI_NEW_STMT);
>        if (result)
>         {
> --
> 2.35.3
>

Re: [PATCH 2/2] gcov: Fix integer types in gen_counter_update()

[Jakub Jelinek]

On Mon, Nov 20, 2023 at 03:33:31PM +0100, Sebastian Huber wrote:
> This change fixes issues like this:
> 
>   gcc.dg/gomp/pr27573.c: In function ‘main._omp_fn.0’:
>   gcc.dg/gomp/pr27573.c:19:1: error: non-trivial conversion in ‘ssa_name’
>      19 | }
>         | ^
>   long int
>   long unsigned int
>   # .MEM_19 = VDEF <.MEM_18>
>   __gcov7.main._omp_fn.0[0] = PROF_time_profile_12;
>   during IPA pass: profile
>   gcc.dg/gomp/pr27573.c:19:1: internal compiler error: verify_gimple failed
> 
> gcc/ChangeLog:
> 
> 	PR middle-end/112634
> 
> 	* tree-profile.cc (gen_assign_counter_update): Cast the unsigned result type of
> 	__atomic_add_fetch() to the signed counter type.
> ---
>  gcc/tree-profile.cc | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/gcc/tree-profile.cc b/gcc/tree-profile.cc
> index 68db09f6189..54938e1d165 100644
> --- a/gcc/tree-profile.cc
> +++ b/gcc/tree-profile.cc
> @@ -284,7 +284,9 @@ gen_assign_counter_update (gimple_stmt_iterator *gsi, gcall *call, tree 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);
> +      gassign *assign = gimple_build_assign (result,
> +					     build_int_cst (TREE_TYPE (result),
> +							    tmp));

This can't be correct.
tmp is a SSA_NAME, so calling build_int_cst on it is not appropriate, the
second argument should be some unsigned HOST_WIDE_INT value.
If result_type is different type from TREE_TYPE (result), but both are
integer types, then you want
      gassign *assing = gimple_build_assign (result, NOP_EXPR, tmp);
or so.

	Jakub

Re: [PATCH 1/2] gcov: Use unshare_expr() in gen_counter_update()

[Dimitar Dimitrov]

On Mon, Nov 20, 2023 at 03:33:30PM +0100, Sebastian Huber wrote:
> This fixes issues like this:
> 
>   gcc/testsuite/gcc.dg/no_profile_instrument_function-attr-1.c: In function 'main':
>   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
>   gcc/testsuite/gcc.dg/no_profile_instrument_function-attr-1.c:19:1: internal compiler error: verify_gimple failed
> 

Hi Sebastian,

This fixes all the regressions I reported for the pru-unknown-elf
target from commit "gcov: Add gen_counter_update()"

Thanks,
Dimitar

Re: [PATCH 2/2] gcov: Fix integer types in gen_counter_update()

[Sebastian Huber]

On 20.11.23 15:56, Jakub Jelinek wrote:
> On Mon, Nov 20, 2023 at 03:33:31PM +0100, Sebastian Huber wrote:
>> This change fixes issues like this:
>>
>>    gcc.dg/gomp/pr27573.c: In function ‘main._omp_fn.0’:
>>    gcc.dg/gomp/pr27573.c:19:1: error: non-trivial conversion in ‘ssa_name’
>>       19 | }
>>          | ^
>>    long int
>>    long unsigned int
>>    # .MEM_19 = VDEF <.MEM_18>
>>    __gcov7.main._omp_fn.0[0] = PROF_time_profile_12;
>>    during IPA pass: profile
>>    gcc.dg/gomp/pr27573.c:19:1: internal compiler error: verify_gimple failed
>>
>> gcc/ChangeLog:
>>
>> 	PR middle-end/112634
>>
>> 	* tree-profile.cc (gen_assign_counter_update): Cast the unsigned result type of
>> 	__atomic_add_fetch() to the signed counter type.
>> ---
>>   gcc/tree-profile.cc | 4 +++-
>>   1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/gcc/tree-profile.cc b/gcc/tree-profile.cc
>> index 68db09f6189..54938e1d165 100644
>> --- a/gcc/tree-profile.cc
>> +++ b/gcc/tree-profile.cc
>> @@ -284,7 +284,9 @@ gen_assign_counter_update (gimple_stmt_iterator *gsi, gcall *call, tree 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);
>> +      gassign *assign = gimple_build_assign (result,
>> +					     build_int_cst (TREE_TYPE (result),
>> +							    tmp));
> This can't be correct.
> tmp is a SSA_NAME, so calling build_int_cst on it is not appropriate, the
> second argument should be some unsigned HOST_WIDE_INT value.
> If result_type is different type from TREE_TYPE (result), but both are
> integer types, then you want
>        gassign *assing = gimple_build_assign (result, NOP_EXPR, tmp);
> or so.

I really don't know what I am doing here, so a lot of guess work is 
involved from my side. The change fixed at least the failing test case. 
When I use the NOP_EXPR

static inline void
gen_assign_counter_update (gimple_stmt_iterator *gsi, gcall *call, tree 
func,
			   tree result, const char *name)
{
   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, NOP_EXPR, tmp);
       gsi_insert_after (gsi, assign, GSI_NEW_STMT);
     }
   else
     gsi_insert_after (gsi, call, GSI_NEW_STMT);
}

I get

gcc -O2 -fopenmp -fprofile-generate 
./gcc/testsuite/gcc.dg/gomp/pr27573.c -S -o -
         .file   "pr27573.c"
./gcc/testsuite/gcc.dg/gomp/pr27573.c: In function ‘main._omp_fn.0’:
./gcc/testsuite/gcc.dg/gomp/pr27573.c:19:1: error: non-register as LHS 
of unary operation
    19 | }
       | ^
# .MEM_19 = VDEF <.MEM_18>
__gcov7.main._omp_fn.0[0] = (long int) PROF_time_profile_12;
during IPA pass: profile
./gcc/testsuite/gcc.dg/gomp/pr27573.c:19:1: internal compiler error: 
verify_gimple failed

-- 
embedded brains GmbH
Herr Sebastian HUBER
Dornierstr. 4
82178 Puchheim
Germany
email: sebastian.huber@embedded-brains.de
phone: +49-89-18 94 741 - 16
fax:   +49-89-18 94 741 - 08

Registergericht: Amtsgericht München
Registernummer: HRB 157899
Vertretungsberechtigte Geschäftsführer: Peter Rasmussen, Thomas Dörfler
Unsere Datenschutzerklärung finden Sie hier:
https://embedded-brains.de/datenschutzerklaerung/

Re: [PATCH 2/2] gcov: Fix integer types in gen_counter_update()

[Jakub Jelinek]

On Tue, Nov 21, 2023 at 10:46:13AM +0100, Sebastian Huber wrote:
> > > --- a/gcc/tree-profile.cc
> > > +++ b/gcc/tree-profile.cc
> > > @@ -284,7 +284,9 @@ gen_assign_counter_update (gimple_stmt_iterator *gsi, gcall *call, tree 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);
> > > +      gassign *assign = gimple_build_assign (result,
> > > +					     build_int_cst (TREE_TYPE (result),
> > > +							    tmp));
> > This can't be correct.
> > tmp is a SSA_NAME, so calling build_int_cst on it is not appropriate, the
> > second argument should be some unsigned HOST_WIDE_INT value.
> > If result_type is different type from TREE_TYPE (result), but both are
> > integer types, then you want
> >        gassign *assing = gimple_build_assign (result, NOP_EXPR, tmp);
> > or so.
> 
> I really don't know what I am doing here, so a lot of guess work is involved
> from my side. The change fixed at least the failing test case. When I use
> the NOP_EXPR
> 
> static inline void
> gen_assign_counter_update (gimple_stmt_iterator *gsi, gcall *call, tree
> func,
> 			   tree result, const char *name)
> {
>   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, NOP_EXPR, tmp);
>       gsi_insert_after (gsi, assign, GSI_NEW_STMT);

Ah, sure, if result is not is_gimple_reg, then one can't use a cast into
that directly, needs to use another statement, one for the cast, one for the
store.
If you know the types are never compatible and result is never is_gimple_reg,
then
        gimple_set_lhs (call, tmp);
        gsi_insert_after (gsi, call, GSI_NEW_STMT);
        gassign *assign
	  = gimple_build_assign (make_ssa_name (TREE_TYPE (result)),
				 NOP_EXPR, tmp);
        gsi_insert_after (gsi, assign, GSI_NEW_STMT);
	assign = gimple_build_assign (result, gimple_assign_lhs (assign));
        gsi_insert_after (gsi, assign, GSI_NEW_STMT);
would do it, if it is sometimes the types are compatible and sometimes they
are not but result never is_gimple_reg, perhaps
        gimple_set_lhs (call, tmp);
        gsi_insert_after (gsi, call, GSI_NEW_STMT);
	if (!useless_type_conversion_p (TREE_TYPE (result), result_type))
	  {
	    gassign *assign
	      = gimple_build_assign (make_ssa_name (TREE_TYPE (result)),
				     NOP_EXPR, tmp);
	    gsi_insert_after (gsi, assign, GSI_NEW_STMT);
	    tmp = gimple_assign_lhs (assign);
	  }
	gassign *assign = gimple_build_assign (result, tmp);
        gsi_insert_after (gsi, assign, GSI_NEW_STMT);
etc.

	Jakub

Re: [PATCH 2/2] gcov: Fix integer types in gen_counter_update()

[Jakub Jelinek]

On Tue, Nov 21, 2023 at 11:07:47AM +0100, Jakub Jelinek wrote:
> > static inline void
> > gen_assign_counter_update (gimple_stmt_iterator *gsi, gcall *call, tree
> > func,
> > 			   tree result, const char *name)
> > {
> >   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, NOP_EXPR, tmp);
> >       gsi_insert_after (gsi, assign, GSI_NEW_STMT);
> 
> Ah, sure, if result is not is_gimple_reg, then one can't use a cast into
> that directly, needs to use another statement, one for the cast, one for the
> store.
> If you know the types are never compatible and result is never is_gimple_reg,
> then
>         gimple_set_lhs (call, tmp);
>         gsi_insert_after (gsi, call, GSI_NEW_STMT);
>         gassign *assign
> 	  = gimple_build_assign (make_ssa_name (TREE_TYPE (result)),
> 				 NOP_EXPR, tmp);
>         gsi_insert_after (gsi, assign, GSI_NEW_STMT);
> 	assign = gimple_build_assign (result, gimple_assign_lhs (assign));
>         gsi_insert_after (gsi, assign, GSI_NEW_STMT);
> would do it

And to answer my own question, seems if result is non-NULL, then it always
has incompatible type and always is ARRAY_REF, i.e. not is_gimple_reg,
because it will have get_gcov_type () which is a signed type and the call
in this case __sync_add_fetch_{4,8} which is unsigned 32-bit or 64-bit
type.  So I'd go with the above.
Another formatting nit:
      tree f = builtin_decl_explicit (TYPE_PRECISION (type) > 32
                                      ? BUILT_IN_ATOMIC_ADD_FETCH_8:
                                      BUILT_IN_ATOMIC_ADD_FETCH_4);
should have been
      tree f = builtin_decl_explicit (TYPE_PRECISION (type) > 32
                                      ? BUILT_IN_ATOMIC_ADD_FETCH_8
                                      : BUILT_IN_ATOMIC_ADD_FETCH_4);
The : shouldn't be at the end of line and there should be space.

	Jakub