Re: [PATCH v2] gcov: Fix integer types in gen_counter_update()
On 23.11.23 18:29, Sebastian Huber wrote: On 23.11.23 09:20, Sebastian Huber wrote: On 23.11.23 09:11, Jiang, Haochen wrote: -Original Message- From: Sebastian Huber Sent: Wednesday, November 22, 2023 10:24 PM To: Christophe Lyon Cc: Jakub Jelinek;gcc-patches@gcc.gnu.org Subject: Re: [PATCH v2] gcov: Fix integer types in gen_counter_update() On 22.11.23 15:22, Christophe Lyon wrote: On Tue, 21 Nov 2023 at 12:22, Sebastian Huber wrote: On 21.11.23 11:46, Jakub Jelinek wrote: On Tue, Nov 21, 2023 at 11:42:06AM +0100, Sebastian Huber wrote: On 21.11.23 11:34, Jakub Jelinek wrote: --- a/gcc/tree-profile.cc +++ b/gcc/tree-profile.cc @@ -281,10 +281,13 @@ gen_assign_counter_update (gimple_stmt_iterator *gsi, gcall *call, tree func, 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); + tree tmp1 = make_temp_ssa_name (result_type, NULL, name); + gimple_set_lhs (call, tmp1); gsi_insert_after (gsi, call, GSI_NEW_STMT); - gassign *assign = gimple_build_assign (result, tmp); + tree tmp2 = make_ssa_name (TREE_TYPE (result)); + gassign *assign = gimple_build_assign (tmp2, NOP_EXPR, tmp1); + gsi_insert_after (gsi, assign, GSI_NEW_STMT); + assign = gimple_build_assign (result, gimple_assign_lhs (assign)); When you use a temporary tmp2 for the lhs of the conversion, you can just use it here, assign = gimple_build_assign (result, tmp2); Ok for trunk with that change. Just a question, could I also use tree tmp2 = make_temp_ssa_name (TREE_TYPE (result), NULL, name); ? This make_temp_ssa_name() is used throughout the file and the new make_ssa_name() would be the first use in this file. Yes. The only difference is that it won't be _234 = (type) something; but PROF_time_profile_234 = (type) something; in the dumps, but sure, consistency is useful. Thanks for your help. I checked in an updated version. Our CI bisected a regression to this commit: Running gcc:gcc.dg/tree-prof/tree-prof.exp ... FAIL: gcc.dg/tree-prof/time-profiler-3.c scan-ipa-dump-times profile "Read tp_first_run: 0" 1 FAIL: gcc.dg/tree-prof/time-profiler-3.c scan-ipa-dump-times profile "Read tp_first_run: 2" 1 (on aarch64) Can you check? Yes, I will have a look at it. The same issue also happened on i386. You can also reproduce that on x86-64 platforms. I was able to reproduce it using a native aarch64 GCC on cfarm185, but I have some difficulties to understand what this test case does actually. Could the problem be caused by some other recent commit? I built this commit: commit 3ef8882adcb1ab5fa535e9e1a2a28ea7c8eeca4f Author: Sebastian Huber Date: Tue Nov 14 21:27:37 2023 +0100 Add TARGET_HAVE_LIBATOMIC Here, the test passes. If I build commit e9b39df9333d565ee06fa65d62bfca4bbcb92e44 (origin/trunk, origin/master, origin/HEAD) Author: Iain Sandoe Date: Tue Nov 21 10:19:29 2023 + testsuite: Update path to intl include. the test fails. To check that my changes caused the problem, I applied the following patches to 3ef8882adcb1ab5fa535e9e1a2a28ea7c8eeca4f (git cherry-pick): Author: Sebastian Huber Date: Sat Oct 21 15:52:15 2023 +0200 gcov: Add gen_counter_update() Author: Sebastian Huber Date: Mon Nov 20 14:48:03 2023 +0100 gcov: Use unshare_expr() in gen_counter_update() Author: Sebastian Huber Date: Mon Nov 20 15:26:38 2023 +0100 gcov: Fix integer types in gen_counter_update() Author: Sebastian Huber Date: Tue Nov 14 21:36:51 2023 +0100 gcov: Improve -fprofile-update=atomic Author: Jakub Jelinek Date: Tue Nov 21 10:49:51 2023 +0100 gcov: Formatting fixes Author: Sebastian Huber Date: Thu Nov 23 06:44:15 2023 + gcov: Do not use atomic ops for -fprofile-update=single With these changes alone on top of 3ef8882adcb1ab5fa535e9e1a2a28ea7c8eeca4f I don't see the regressions: Executing on host: /home/sh/b-gcc/gcc/xgcc -B/home/sh/b-gcc/gcc/ /home/sh/gcc/gcc/testsuite/gcc.dg/tree-prof/time-profiler-3.c -fdiagnostics-plain-output -O2 -fdump-ipa-profile -fprofile-update=atomic -fprofile-generate -D_PROFILE_GENERATE -dumpbase-ext .x01 -lm -o /home/sh/b-gcc/gcc/testsuite/gcc5/time-profiler-3.x01 (timeout = 300) spawn -ignore SIGHUP /home/sh/b-gcc/gcc/xgcc -B/home/sh/b-gcc/gcc/ /home/sh/gcc/gcc/testsuite/gcc.dg/tree-prof/time-profiler-3.c -fdiagnostics-plain-output -O2 -fdump-ipa-profile -fprofile-update=atomic -fprofile-generate -D_PROFILE_GENERATE -dumpbase-ext .x01 -lm -o /home/sh/b-gcc/gcc/testsuite/gcc5/time-profiler-3.x01 PASS: gcc.dg/tree-prof/time-profiler-3.c compilation, -fprofile-generate -D_PROFILE_GENERATE Setting LD_LIBRARY_PATH to :/home/sh/b-gcc/gcc:/home/sh/b-gcc/aarch64-unknown-linux-gnu/./libatomic/.libs::/home/sh/b-gcc/gcc:/home/sh/b-gcc/aarch64-unknown-linux-gnu/./libatomi
Re: [PATCH v2] gcov: Fix integer types in gen_counter_update()
On 23.11.23 09:20, Sebastian Huber wrote: On 23.11.23 09:11, Jiang, Haochen wrote: -Original Message- From: Sebastian Huber Sent: Wednesday, November 22, 2023 10:24 PM To: Christophe Lyon Cc: Jakub Jelinek;gcc-patches@gcc.gnu.org Subject: Re: [PATCH v2] gcov: Fix integer types in gen_counter_update() On 22.11.23 15:22, Christophe Lyon wrote: On Tue, 21 Nov 2023 at 12:22, Sebastian Huber wrote: On 21.11.23 11:46, Jakub Jelinek wrote: On Tue, Nov 21, 2023 at 11:42:06AM +0100, Sebastian Huber wrote: On 21.11.23 11:34, Jakub Jelinek wrote: --- a/gcc/tree-profile.cc +++ b/gcc/tree-profile.cc @@ -281,10 +281,13 @@ gen_assign_counter_update (gimple_stmt_iterator *gsi, gcall *call, tree func, 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); + tree tmp1 = make_temp_ssa_name (result_type, NULL, name); + gimple_set_lhs (call, tmp1); gsi_insert_after (gsi, call, GSI_NEW_STMT); - gassign *assign = gimple_build_assign (result, tmp); + tree tmp2 = make_ssa_name (TREE_TYPE (result)); + gassign *assign = gimple_build_assign (tmp2, NOP_EXPR, tmp1); + gsi_insert_after (gsi, assign, GSI_NEW_STMT); + assign = gimple_build_assign (result, gimple_assign_lhs (assign)); When you use a temporary tmp2 for the lhs of the conversion, you can just use it here, assign = gimple_build_assign (result, tmp2); Ok for trunk with that change. Just a question, could I also use tree tmp2 = make_temp_ssa_name (TREE_TYPE (result), NULL, name); ? This make_temp_ssa_name() is used throughout the file and the new make_ssa_name() would be the first use in this file. Yes. The only difference is that it won't be _234 = (type) something; but PROF_time_profile_234 = (type) something; in the dumps, but sure, consistency is useful. Thanks for your help. I checked in an updated version. Our CI bisected a regression to this commit: Running gcc:gcc.dg/tree-prof/tree-prof.exp ... FAIL: gcc.dg/tree-prof/time-profiler-3.c scan-ipa-dump-times profile "Read tp_first_run: 0" 1 FAIL: gcc.dg/tree-prof/time-profiler-3.c scan-ipa-dump-times profile "Read tp_first_run: 2" 1 (on aarch64) Can you check? Yes, I will have a look at it. The same issue also happened on i386. You can also reproduce that on x86-64 platforms. I was able to reproduce it using a native aarch64 GCC on cfarm185, but I have some difficulties to understand what this test case does actually. Could the problem be caused by some other recent commit? I built this commit: commit 3ef8882adcb1ab5fa535e9e1a2a28ea7c8eeca4f Author: Sebastian Huber Date: Tue Nov 14 21:27:37 2023 +0100 Add TARGET_HAVE_LIBATOMIC Here, the test passes. If I build commit e9b39df9333d565ee06fa65d62bfca4bbcb92e44 (origin/trunk, origin/master, origin/HEAD) Author: Iain Sandoe Date: Tue Nov 21 10:19:29 2023 + testsuite: Update path to intl include. the test fails. To check that my changes caused the problem, I applied the following patches to 3ef8882adcb1ab5fa535e9e1a2a28ea7c8eeca4f (git cherry-pick): Author: Sebastian Huber Date: Sat Oct 21 15:52:15 2023 +0200 gcov: Add gen_counter_update() Author: Sebastian Huber Date: Mon Nov 20 14:48:03 2023 +0100 gcov: Use unshare_expr() in gen_counter_update() Author: Sebastian Huber Date: Mon Nov 20 15:26:38 2023 +0100 gcov: Fix integer types in gen_counter_update() Author: Sebastian Huber Date: Tue Nov 14 21:36:51 2023 +0100 gcov: Improve -fprofile-update=atomic Author: Jakub Jelinek Date: Tue Nov 21 10:49:51 2023 +0100 gcov: Formatting fixes Author: Sebastian Huber Date: Thu Nov 23 06:44:15 2023 + gcov: Do not use atomic ops for -fprofile-update=single With these changes alone on top of 3ef8882adcb1ab5fa535e9e1a2a28ea7c8eeca4f I don't see the regressions: Executing on host: /home/sh/b-gcc/gcc/xgcc -B/home/sh/b-gcc/gcc/ /home/sh/gcc/gcc/testsuite/gcc.dg/tree-prof/time-profiler-3.c -fdiagnostics-plain-output -O2 -fdump-ipa-profile -fprofile-update=atomic -fprofile-generate -D_PROFILE_GENERATE -dumpbase-ext .x01 -lm -o /home/sh/b-gcc/gcc/testsuite/gcc5/time-profiler-3.x01(timeout = 300) spawn -ignore SIGHUP /home/sh/b-gcc/gcc/xgcc -B/home/sh/b-gcc/gcc/ /home/sh/gcc/gcc/testsuite/gcc.dg/tree-prof/time-profiler-3.c -fdiagnostics-plain-output -O2 -fdump-ipa-profile -fprofile-update=atomic -fprofile-generate -D_PROFILE_GENERATE -dumpbase-ext .x01 -lm -o /home/sh/b-gcc/gcc/testsuite/gcc5/time-profiler-3.x01 PASS: gcc.dg/tree-prof/time-profiler-3.c compilation, -fprofile-generate -D_PROFILE_GENERATE Setting LD_LIBRARY_PATH to :/home/sh/b-gcc/gcc:/home/sh/b-gcc/aarch64-unknown-linux-gnu/./libatomic/.libs::/home/sh/b-gcc/gcc:/home/sh/b-gcc/aarch64-unknown-linux-gnu/./libatomic/.libs:/home/sh/b-gcc/aarch64-unknown-linux-gnu/lib
Re: [PATCH v2] gcov: Fix integer types in gen_counter_update()
On 23.11.23 09:11, Jiang, Haochen wrote: -Original Message- From: Sebastian Huber Sent: Wednesday, November 22, 2023 10:24 PM To: Christophe Lyon Cc: Jakub Jelinek;gcc-patches@gcc.gnu.org Subject: Re: [PATCH v2] gcov: Fix integer types in gen_counter_update() On 22.11.23 15:22, Christophe Lyon wrote: On Tue, 21 Nov 2023 at 12:22, Sebastian Huber wrote: On 21.11.23 11:46, Jakub Jelinek wrote: On Tue, Nov 21, 2023 at 11:42:06AM +0100, Sebastian Huber wrote: On 21.11.23 11:34, Jakub Jelinek wrote: --- a/gcc/tree-profile.cc +++ b/gcc/tree-profile.cc @@ -281,10 +281,13 @@ gen_assign_counter_update (gimple_stmt_iterator *gsi, gcall *call, tree func, 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); + tree tmp1 = make_temp_ssa_name (result_type, NULL, name); + gimple_set_lhs (call, tmp1); gsi_insert_after (gsi, call, GSI_NEW_STMT); - gassign *assign = gimple_build_assign (result, tmp); + tree tmp2 = make_ssa_name (TREE_TYPE (result)); + gassign *assign = gimple_build_assign (tmp2, NOP_EXPR, tmp1); + gsi_insert_after (gsi, assign, GSI_NEW_STMT); + assign = gimple_build_assign (result, gimple_assign_lhs (assign)); When you use a temporary tmp2 for the lhs of the conversion, you can just use it here, assign = gimple_build_assign (result, tmp2); Ok for trunk with that change. Just a question, could I also use tree tmp2 = make_temp_ssa_name (TREE_TYPE (result), NULL, name); ? This make_temp_ssa_name() is used throughout the file and the new make_ssa_name() would be the first use in this file. Yes. The only difference is that it won't be _234 = (type) something; but PROF_time_profile_234 = (type) something; in the dumps, but sure, consistency is useful. Thanks for your help. I checked in an updated version. Our CI bisected a regression to this commit: Running gcc:gcc.dg/tree-prof/tree-prof.exp ... FAIL: gcc.dg/tree-prof/time-profiler-3.c scan-ipa-dump-times profile "Read tp_first_run: 0" 1 FAIL: gcc.dg/tree-prof/time-profiler-3.c scan-ipa-dump-times profile "Read tp_first_run: 2" 1 (on aarch64) Can you check? Yes, I will have a look at it. The same issue also happened on i386. You can also reproduce that on x86-64 platforms. I was able to reproduce it using a native aarch64 GCC on cfarm185, but I have some difficulties to understand what this test case does actually. -- embedded brains GmbH Herr Sebastian HUBER Dornierstr. 4 82178 Puchheim Germany email: sebastian.hu...@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 v2] gcov: Fix integer types in gen_counter_update()
> -Original Message- > From: Sebastian Huber > Sent: Wednesday, November 22, 2023 10:24 PM > To: Christophe Lyon > Cc: Jakub Jelinek ; gcc-patches@gcc.gnu.org > Subject: Re: [PATCH v2] gcov: Fix integer types in gen_counter_update() > > On 22.11.23 15:22, Christophe Lyon wrote: > > On Tue, 21 Nov 2023 at 12:22, Sebastian Huber > > wrote: > >> On 21.11.23 11:46, Jakub Jelinek wrote: > >>> On Tue, Nov 21, 2023 at 11:42:06AM +0100, Sebastian Huber wrote: > >>>> On 21.11.23 11:34, Jakub Jelinek wrote: > >>>>>> --- a/gcc/tree-profile.cc > >>>>>> +++ b/gcc/tree-profile.cc > >>>>>> @@ -281,10 +281,13 @@ gen_assign_counter_update > (gimple_stmt_iterator *gsi, gcall *call, tree func, > >>>>>> 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); > >>>>>> + tree tmp1 = make_temp_ssa_name (result_type, NULL, name); > >>>>>> + gimple_set_lhs (call, tmp1); > >>>>>> gsi_insert_after (gsi, call, GSI_NEW_STMT); > >>>>>> - gassign *assign = gimple_build_assign (result, tmp); > >>>>>> + tree tmp2 = make_ssa_name (TREE_TYPE (result)); > >>>>>> + gassign *assign = gimple_build_assign (tmp2, NOP_EXPR, tmp1); > >>>>>> + gsi_insert_after (gsi, assign, GSI_NEW_STMT); > >>>>>> + assign = gimple_build_assign (result, gimple_assign_lhs > >>>>>> (assign)); > >>>>> When you use a temporary tmp2 for the lhs of the conversion, you can > just > >>>>> use it here, > >>>>> assign = gimple_build_assign (result, tmp2); > >>>>> > >>>>> Ok for trunk with that change. > >>>> Just a question, could I also use > >>>> > >>>> tree tmp2 = make_temp_ssa_name (TREE_TYPE (result), NULL, name); > >>>> > >>>> ? > >>>> > >>>> This make_temp_ssa_name() is used throughout the file and the new > >>>> make_ssa_name() would be the first use in this file. > >>> Yes. The only difference is that it won't be _234 = (type) something; > >>> but PROF_time_profile_234 = (type) something; in the dumps, but sure, > >>> consistency is useful. > >> Thanks for your help. I checked in an updated version. > >> > > Our CI bisected a regression to this commit: > > Running gcc:gcc.dg/tree-prof/tree-prof.exp ... > > FAIL: gcc.dg/tree-prof/time-profiler-3.c scan-ipa-dump-times profile > > "Read tp_first_run: 0" 1 > > FAIL: gcc.dg/tree-prof/time-profiler-3.c scan-ipa-dump-times profile > > "Read tp_first_run: 2" 1 > > > > (on aarch64) > > > > Can you check? > > Yes, I will have a look at it. The same issue also happened on i386. You can also reproduce that on x86-64 platforms. Thx, Haochen > > -- > embedded brains GmbH > Herr Sebastian HUBER > Dornierstr. 4 > 82178 Puchheim > Germany > email: sebastian.hu...@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 v2] gcov: Fix integer types in gen_counter_update()
On 22.11.23 15:22, Christophe Lyon wrote: On Tue, 21 Nov 2023 at 12:22, Sebastian Huber wrote: On 21.11.23 11:46, Jakub Jelinek wrote: On Tue, Nov 21, 2023 at 11:42:06AM +0100, Sebastian Huber wrote: On 21.11.23 11:34, Jakub Jelinek wrote: --- a/gcc/tree-profile.cc +++ b/gcc/tree-profile.cc @@ -281,10 +281,13 @@ gen_assign_counter_update (gimple_stmt_iterator *gsi, gcall *call, tree func, 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); + tree tmp1 = make_temp_ssa_name (result_type, NULL, name); + gimple_set_lhs (call, tmp1); gsi_insert_after (gsi, call, GSI_NEW_STMT); - gassign *assign = gimple_build_assign (result, tmp); + tree tmp2 = make_ssa_name (TREE_TYPE (result)); + gassign *assign = gimple_build_assign (tmp2, NOP_EXPR, tmp1); + gsi_insert_after (gsi, assign, GSI_NEW_STMT); + assign = gimple_build_assign (result, gimple_assign_lhs (assign)); When you use a temporary tmp2 for the lhs of the conversion, you can just use it here, assign = gimple_build_assign (result, tmp2); Ok for trunk with that change. Just a question, could I also use tree tmp2 = make_temp_ssa_name (TREE_TYPE (result), NULL, name); ? This make_temp_ssa_name() is used throughout the file and the new make_ssa_name() would be the first use in this file. Yes. The only difference is that it won't be _234 = (type) something; but PROF_time_profile_234 = (type) something; in the dumps, but sure, consistency is useful. Thanks for your help. I checked in an updated version. Our CI bisected a regression to this commit: Running gcc:gcc.dg/tree-prof/tree-prof.exp ... FAIL: gcc.dg/tree-prof/time-profiler-3.c scan-ipa-dump-times profile "Read tp_first_run: 0" 1 FAIL: gcc.dg/tree-prof/time-profiler-3.c scan-ipa-dump-times profile "Read tp_first_run: 2" 1 (on aarch64) Can you check? Yes, I will have a look at it. -- embedded brains GmbH Herr Sebastian HUBER Dornierstr. 4 82178 Puchheim Germany email: sebastian.hu...@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 v2] gcov: Fix integer types in gen_counter_update()
Hi, On Tue, 21 Nov 2023 at 12:22, Sebastian Huber wrote: > > On 21.11.23 11:46, Jakub Jelinek wrote: > > On Tue, Nov 21, 2023 at 11:42:06AM +0100, Sebastian Huber wrote: > >> > >> On 21.11.23 11:34, Jakub Jelinek wrote: > --- a/gcc/tree-profile.cc > +++ b/gcc/tree-profile.cc > @@ -281,10 +281,13 @@ gen_assign_counter_update (gimple_stmt_iterator > *gsi, gcall *call, tree func, > 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); > + tree tmp1 = make_temp_ssa_name (result_type, NULL, name); > + gimple_set_lhs (call, tmp1); > gsi_insert_after (gsi, call, GSI_NEW_STMT); > - gassign *assign = gimple_build_assign (result, tmp); > + tree tmp2 = make_ssa_name (TREE_TYPE (result)); > + gassign *assign = gimple_build_assign (tmp2, NOP_EXPR, tmp1); > + gsi_insert_after (gsi, assign, GSI_NEW_STMT); > + assign = gimple_build_assign (result, gimple_assign_lhs (assign)); > >>> When you use a temporary tmp2 for the lhs of the conversion, you can just > >>> use it here, > >>> assign = gimple_build_assign (result, tmp2); > >>> > >>> Ok for trunk with that change. > >> Just a question, could I also use > >> > >> tree tmp2 = make_temp_ssa_name (TREE_TYPE (result), NULL, name); > >> > >> ? > >> > >> This make_temp_ssa_name() is used throughout the file and the new > >> make_ssa_name() would be the first use in this file. > > Yes. The only difference is that it won't be _234 = (type) something; > > but PROF_time_profile_234 = (type) something; in the dumps, but sure, > > consistency is useful. > > Thanks for your help. I checked in an updated version. > Our CI bisected a regression to this commit: Running gcc:gcc.dg/tree-prof/tree-prof.exp ... FAIL: gcc.dg/tree-prof/time-profiler-3.c scan-ipa-dump-times profile "Read tp_first_run: 0" 1 FAIL: gcc.dg/tree-prof/time-profiler-3.c scan-ipa-dump-times profile "Read tp_first_run: 2" 1 (on aarch64) Can you check? Thanks, Christophe > -- > embedded brains GmbH > Herr Sebastian HUBER > Dornierstr. 4 > 82178 Puchheim > Germany > email: sebastian.hu...@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 v2] gcov: Fix integer types in gen_counter_update()
On 21.11.23 11:46, Jakub Jelinek wrote: On Tue, Nov 21, 2023 at 11:42:06AM +0100, Sebastian Huber wrote: On 21.11.23 11:34, Jakub Jelinek wrote: --- a/gcc/tree-profile.cc +++ b/gcc/tree-profile.cc @@ -281,10 +281,13 @@ gen_assign_counter_update (gimple_stmt_iterator *gsi, gcall *call, tree func, 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); + tree tmp1 = make_temp_ssa_name (result_type, NULL, name); + gimple_set_lhs (call, tmp1); gsi_insert_after (gsi, call, GSI_NEW_STMT); - gassign *assign = gimple_build_assign (result, tmp); + tree tmp2 = make_ssa_name (TREE_TYPE (result)); + gassign *assign = gimple_build_assign (tmp2, NOP_EXPR, tmp1); + gsi_insert_after (gsi, assign, GSI_NEW_STMT); + assign = gimple_build_assign (result, gimple_assign_lhs (assign)); When you use a temporary tmp2 for the lhs of the conversion, you can just use it here, assign = gimple_build_assign (result, tmp2); Ok for trunk with that change. Just a question, could I also use tree tmp2 = make_temp_ssa_name (TREE_TYPE (result), NULL, name); ? This make_temp_ssa_name() is used throughout the file and the new make_ssa_name() would be the first use in this file. Yes. The only difference is that it won't be _234 = (type) something; but PROF_time_profile_234 = (type) something; in the dumps, but sure, consistency is useful. Thanks for your help. I checked in an updated version. -- embedded brains GmbH Herr Sebastian HUBER Dornierstr. 4 82178 Puchheim Germany email: sebastian.hu...@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 v2] gcov: Fix integer types in gen_counter_update()
On Tue, Nov 21, 2023 at 11:42:06AM +0100, Sebastian Huber wrote: > > > On 21.11.23 11:34, Jakub Jelinek wrote: > > > --- a/gcc/tree-profile.cc > > > +++ b/gcc/tree-profile.cc > > > @@ -281,10 +281,13 @@ gen_assign_counter_update (gimple_stmt_iterator > > > *gsi, gcall *call, tree func, > > > 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); > > > + tree tmp1 = make_temp_ssa_name (result_type, NULL, name); > > > + gimple_set_lhs (call, tmp1); > > > gsi_insert_after (gsi, call, GSI_NEW_STMT); > > > - gassign *assign = gimple_build_assign (result, tmp); > > > + tree tmp2 = make_ssa_name (TREE_TYPE (result)); > > > + gassign *assign = gimple_build_assign (tmp2, NOP_EXPR, tmp1); > > > + gsi_insert_after (gsi, assign, GSI_NEW_STMT); > > > + assign = gimple_build_assign (result, gimple_assign_lhs (assign)); > > When you use a temporary tmp2 for the lhs of the conversion, you can just > > use it here, > >assign = gimple_build_assign (result, tmp2); > > > > Ok for trunk with that change. > > Just a question, could I also use > > tree tmp2 = make_temp_ssa_name (TREE_TYPE (result), NULL, name); > > ? > > This make_temp_ssa_name() is used throughout the file and the new > make_ssa_name() would be the first use in this file. Yes. The only difference is that it won't be _234 = (type) something; but PROF_time_profile_234 = (type) something; in the dumps, but sure, consistency is useful. Jakub
Re: [PATCH v2] gcov: Fix integer types in gen_counter_update()
On 21.11.23 11:34, Jakub Jelinek wrote: --- a/gcc/tree-profile.cc +++ b/gcc/tree-profile.cc @@ -281,10 +281,13 @@ gen_assign_counter_update (gimple_stmt_iterator *gsi, gcall *call, tree func, 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); + tree tmp1 = make_temp_ssa_name (result_type, NULL, name); + gimple_set_lhs (call, tmp1); gsi_insert_after (gsi, call, GSI_NEW_STMT); - gassign *assign = gimple_build_assign (result, tmp); + tree tmp2 = make_ssa_name (TREE_TYPE (result)); + gassign *assign = gimple_build_assign (tmp2, NOP_EXPR, tmp1); + gsi_insert_after (gsi, assign, GSI_NEW_STMT); + assign = gimple_build_assign (result, gimple_assign_lhs (assign)); When you use a temporary tmp2 for the lhs of the conversion, you can just use it here, assign = gimple_build_assign (result, tmp2); Ok for trunk with that change. Just a question, could I also use tree tmp2 = make_temp_ssa_name (TREE_TYPE (result), NULL, name); ? This make_temp_ssa_name() is used throughout the file and the new make_ssa_name() would be the first use in this file. -- embedded brains GmbH Herr Sebastian HUBER Dornierstr. 4 82178 Puchheim Germany email: sebastian.hu...@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 v2] gcov: Fix integer types in gen_counter_update()
On Tue, Nov 21, 2023 at 11:29:58AM +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. > (gen_counter_update): Fix formatting. > --- a/gcc/tree-profile.cc > +++ b/gcc/tree-profile.cc > @@ -281,10 +281,13 @@ gen_assign_counter_update (gimple_stmt_iterator *gsi, > gcall *call, tree func, >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); > + tree tmp1 = make_temp_ssa_name (result_type, NULL, name); > + gimple_set_lhs (call, tmp1); >gsi_insert_after (gsi, call, GSI_NEW_STMT); > - gassign *assign = gimple_build_assign (result, tmp); > + tree tmp2 = make_ssa_name (TREE_TYPE (result)); > + gassign *assign = gimple_build_assign (tmp2, NOP_EXPR, tmp1); > + gsi_insert_after (gsi, assign, GSI_NEW_STMT); > + assign = gimple_build_assign (result, gimple_assign_lhs (assign)); When you use a temporary tmp2 for the lhs of the conversion, you can just use it here, assign = gimple_build_assign (result, tmp2); Ok for trunk with that change. >gsi_insert_after (gsi, assign, GSI_NEW_STMT); > } >else > @@ -309,8 +312,8 @@ gen_counter_update (gimple_stmt_iterator *gsi, tree > counter, tree result, > { >/* __atomic_fetch_add (&counter, 1, MEMMODEL_RELAXED); */ >tree f = builtin_decl_explicit (TYPE_PRECISION (type) > 32 > - ? BUILT_IN_ATOMIC_ADD_FETCH_8: > - BUILT_IN_ATOMIC_ADD_FETCH_4); > + ? 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); > } Jakub
[PATCH v2] gcov: Fix integer types in gen_counter_update()
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. (gen_counter_update): Fix formatting. --- v2: Use NOP_EXPR to do the cast. Fix formatting. gcc/tree-profile.cc | 13 - 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/gcc/tree-profile.cc b/gcc/tree-profile.cc index f12b374ca27..ff95f8ef7cd 100644 --- a/gcc/tree-profile.cc +++ b/gcc/tree-profile.cc @@ -281,10 +281,13 @@ gen_assign_counter_update (gimple_stmt_iterator *gsi, gcall *call, tree func, 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); + tree tmp1 = make_temp_ssa_name (result_type, NULL, name); + gimple_set_lhs (call, tmp1); gsi_insert_after (gsi, call, GSI_NEW_STMT); - gassign *assign = gimple_build_assign (result, tmp); + tree tmp2 = make_ssa_name (TREE_TYPE (result)); + gassign *assign = gimple_build_assign (tmp2, NOP_EXPR, tmp1); + 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); } else @@ -309,8 +312,8 @@ gen_counter_update (gimple_stmt_iterator *gsi, tree counter, tree result, { /* __atomic_fetch_add (&counter, 1, MEMMODEL_RELAXED); */ tree f = builtin_decl_explicit (TYPE_PRECISION (type) > 32 - ? BUILT_IN_ATOMIC_ADD_FETCH_8: - BUILT_IN_ATOMIC_ADD_FETCH_4); + ? 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); } -- 2.35.3