| Message ID | 20231120143331.33102-2-sebastian.huber@embedded-brains.de |
|---|---|
| State | New |
| Headers | show |
| Series | [1/2] gcov: Use unshare_expr() in gen_counter_update() | expand |
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
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
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
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
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
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(-)