[2/2] gcov: Fix integer types in gen_counter_update()
Checks
Context |
Check |
Description |
linaro-tcwg-bot/tcwg_gcc_build--master-arm |
success
|
Testing passed
|
linaro-tcwg-bot/tcwg_gcc_build--master-aarch64 |
fail
|
Testing failed
|
Commit Message
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(-)
Comments
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
@@ -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