| Message ID | 56B88E34.2050400@mentor.com |
|---|---|
| State | New |
| Headers | show |
On Mon, Feb 08, 2016 at 01:46:44PM +0100, Tom de Vries wrote: > [ The pass before pass_omp_simd_clone is pass_dispatcher_calls. It has a > function create_target_clone, similar to simd_clone_create, with a > node.defition and !node.defition part. The !node.defition part does not call > 'symtab->call_cgraph_insertion_hooks (new_node)'. ] I'll defer to Honza or Richi if it is ok not to call cgraph insertion hooks at this point (and since when they can be avoided), or what else should be done. The patch could be ok even for 6.0, not just stage1, if they are ok with it (or propose some other change). > Don't call call_cgraph_insertion_hooks in simd_clone_create > > 2016-02-08 Tom de Vries <tom@codesourcery.com> > > PR lto/67709 > * omp-low.c (simd_clone_create): Remove call to > symtab->call_cgraph_insertion_hooks. > > * testsuite/libgomp.fortran/declare-simd-4.f90: New test. Jakub
On 08/02/16 13:54, Jakub Jelinek wrote: > On Mon, Feb 08, 2016 at 01:46:44PM +0100, Tom de Vries wrote: >> [ The pass before pass_omp_simd_clone is pass_dispatcher_calls. It has a >> function create_target_clone, similar to simd_clone_create, with a >> node.defition and !node.defition part. The !node.defition part does not call >> 'symtab->call_cgraph_insertion_hooks (new_node)'. ] > > I'll defer to Honza or Richi if it is ok not to call cgraph insertion hooks > at this point (and since when they can be avoided), or what else should be > done. > > The patch could be ok even for 6.0, not just stage1, if they are ok with it > (or propose some other change). > Ping (Given that Jakub suggested this or an alternative patch might be included in 6.0 stage4). Original submission at https://gcc.gnu.org/ml/gcc-patches/2016-02/msg00543.html . Thanks, - Tom >> Don't call call_cgraph_insertion_hooks in simd_clone_create >> >> 2016-02-08 Tom de Vries <tom@codesourcery.com> >> >> PR lto/67709 >> * omp-low.c (simd_clone_create): Remove call to >> symtab->call_cgraph_insertion_hooks. >> >> * testsuite/libgomp.fortran/declare-simd-4.f90: New test. > > Jakub >
> On 08/02/16 13:54, Jakub Jelinek wrote: > >On Mon, Feb 08, 2016 at 01:46:44PM +0100, Tom de Vries wrote: > >>[ The pass before pass_omp_simd_clone is pass_dispatcher_calls. It has a > >>function create_target_clone, similar to simd_clone_create, with a > >>node.defition and !node.defition part. The !node.defition part does not call > >>'symtab->call_cgraph_insertion_hooks (new_node)'. ] > > > >I'll defer to Honza or Richi if it is ok not to call cgraph insertion hooks > >at this point (and since when they can be avoided), or what else should be > >done. > > > >The patch could be ok even for 6.0, not just stage1, if they are ok with it > >(or propose some other change). > > > > Ping (Given that Jakub suggested this or an alternative patch might > be included in 6.0 stage4). > > Original submission at > https://gcc.gnu.org/ml/gcc-patches/2016-02/msg00543.html . OK, so the ICE is that you intorduce new deifnition but you set in_other_partition and do not really introudce gimple body for it? In that case we should indeed not call creation hooks, because these are used only when new function is introduced to a given partition. Patch looks OK to me thus. Honza. > > Thanks, > - Tom > > >>Don't call call_cgraph_insertion_hooks in simd_clone_create > >> > >>2016-02-08 Tom de Vries <tom@codesourcery.com> > >> > >> PR lto/67709 > >> * omp-low.c (simd_clone_create): Remove call to > >> symtab->call_cgraph_insertion_hooks. > >> > >> * testsuite/libgomp.fortran/declare-simd-4.f90: New test. > > > > Jakub > >
On 16/02/16 03:22, Jan Hubicka wrote: >> On 08/02/16 13:54, Jakub Jelinek wrote: >>> On Mon, Feb 08, 2016 at 01:46:44PM +0100, Tom de Vries wrote: >>>> [ The pass before pass_omp_simd_clone is pass_dispatcher_calls. It has a >>>> function create_target_clone, similar to simd_clone_create, with a >>>> node.defition and !node.defition part. The !node.defition part does not call >>>> 'symtab->call_cgraph_insertion_hooks (new_node)'. ] >>> >>> I'll defer to Honza or Richi if it is ok not to call cgraph insertion hooks >>> at this point (and since when they can be avoided), or what else should be >>> done. >>> >>> The patch could be ok even for 6.0, not just stage1, if they are ok with it >>> (or propose some other change). >>> >> >> Ping (Given that Jakub suggested this or an alternative patch might >> be included in 6.0 stage4). >> >> Original submission at >> https://gcc.gnu.org/ml/gcc-patches/2016-02/msg00543.html . > > OK, so the ICE is that you intorduce new deifnition but you set in_other_partition > and do not really introudce gimple body for it? > In that case we should indeed not call creation hooks, because these are used only > when new function is introduced to a given partition. > > Patch looks OK to me thus. Committed to trunk. AFAIU, it's not a release regression given that: - this has failed since 4.9.0, and - the test-case is not supported in 4.8, so we're not required to fix it in 4.9 and 5 branches. But, the test-case does fail in 5 and 4.9 branches, and the fix seems simple enough to not have the risk of introducing breakage in the release branches. OK for 4.9/5 branch? Thanks, - Tom > Honza. >> >> Thanks, >> - Tom >> >>>> Don't call call_cgraph_insertion_hooks in simd_clone_create >>>> >>>> 2016-02-08 Tom de Vries <tom@codesourcery.com> >>>> >>>> PR lto/67709 >>>> * omp-low.c (simd_clone_create): Remove call to >>>> symtab->call_cgraph_insertion_hooks. >>>> >>>> * testsuite/libgomp.fortran/declare-simd-4.f90: New test. >>> >>> Jakub >>>
On Tue, Feb 16, 2016 at 10:56:58AM +0100, Tom de Vries wrote: > AFAIU, it's not a release regression given that: > - this has failed since 4.9.0, and > - the test-case is not supported in 4.8, > so we're not required to fix it in 4.9 and 5 branches. > > But, the test-case does fail in 5 and 4.9 branches, and the fix seems simple > enough to not have the risk of introducing breakage in the release branches. > > OK for 4.9/5 branch? Okay. > >>>>2016-02-08 Tom de Vries <tom@codesourcery.com> > >>>> > >>>> PR lto/67709 > >>>> * omp-low.c (simd_clone_create): Remove call to > >>>> symtab->call_cgraph_insertion_hooks. > >>>> > >>>> * testsuite/libgomp.fortran/declare-simd-4.f90: New test. Jakub
On 16/02/16 11:04, Jakub Jelinek wrote: > On Tue, Feb 16, 2016 at 10:56:58AM +0100, Tom de Vries wrote: >> >AFAIU, it's not a release regression given that: >> >- this has failed since 4.9.0, and >> >- the test-case is not supported in 4.8, >> >so we're not required to fix it in 4.9 and 5 branches. >> > >> >But, the test-case does fail in 5 and 4.9 branches, and the fix seems simple >> >enough to not have the risk of introducing breakage in the release branches. >> > >> >OK for 4.9/5 branch? > Okay. > Hmm, it doesn't apply cleanly. The trunk version of simd_clone_create also contains: 'Fix ICE for SIMD clones usage in LTO' ( https://gcc.gnu.org/ml/gcc-patches/2015-10/msg00434.html ), which adds the 'in_other_partition' propagation in simd_clone_create. That's also not a release regression (reproduces with 4.9.0 release, not supported in 4.8), and still reproduces on 4.9 and 5 branches. OK to backport 'Fix ICE for SIMD clones usage in LTO' to 4.9/5 branch? Thanks, - Tom
On Tue, Feb 16, 2016 at 12:10:29PM +0100, Tom de Vries wrote: > On 16/02/16 11:04, Jakub Jelinek wrote: > >On Tue, Feb 16, 2016 at 10:56:58AM +0100, Tom de Vries wrote: > >>>AFAIU, it's not a release regression given that: > >>>- this has failed since 4.9.0, and > >>>- the test-case is not supported in 4.8, > >>>so we're not required to fix it in 4.9 and 5 branches. > >>> > >>>But, the test-case does fail in 5 and 4.9 branches, and the fix seems simple > >>>enough to not have the risk of introducing breakage in the release branches. > >>> > >>>OK for 4.9/5 branch? > >Okay. > > > > Hmm, it doesn't apply cleanly. > > The trunk version of simd_clone_create also contains: 'Fix ICE for SIMD > clones usage in LTO' ( > https://gcc.gnu.org/ml/gcc-patches/2015-10/msg00434.html ), which adds the > 'in_other_partition' propagation in simd_clone_create. > > That's also not a release regression (reproduces with 4.9.0 release, not > supported in 4.8), and still reproduces on 4.9 and 5 branches. > > OK to backport 'Fix ICE for SIMD clones usage in LTO' to 4.9/5 branch? Ok. Jakub
On 16/02/16 12:11, Jakub Jelinek wrote: > On Tue, Feb 16, 2016 at 12:10:29PM +0100, Tom de Vries wrote: >> >On 16/02/16 11:04, Jakub Jelinek wrote: >>> > >On Tue, Feb 16, 2016 at 10:56:58AM +0100, Tom de Vries wrote: >>>>> > >>>AFAIU, it's not a release regression given that: >>>>> > >>>- this has failed since 4.9.0, and >>>>> > >>>- the test-case is not supported in 4.8, >>>>> > >>>so we're not required to fix it in 4.9 and 5 branches. >>>>> > >>> >>>>> > >>>But, the test-case does fail in 5 and 4.9 branches, and the fix seems simple >>>>> > >>>enough to not have the risk of introducing breakage in the release branches. >>>>> > >>> >>>>> > >>>OK for 4.9/5 branch? >>> > >Okay. >>> > > >> > >> >Hmm, it doesn't apply cleanly. >> > >> >The trunk version of simd_clone_create also contains: 'Fix ICE for SIMD >> >clones usage in LTO' ( >> >https://gcc.gnu.org/ml/gcc-patches/2015-10/msg00434.html ), which adds the >> >'in_other_partition' propagation in simd_clone_create. >> > >> >That's also not a release regression (reproduces with 4.9.0 release, not >> >supported in 4.8), and still reproduces on 4.9 and 5 branches. >> > >> >OK to backport 'Fix ICE for SIMD clones usage in LTO' to 4.9/5 branch? > Ok. Committed both patches to 4.9 and 5 branches. In order to run testsuite/libgomp.fortran/declare-simd-4.f90 with the 4.9 branch build, I needed in addition: - r212268 https://gcc.gnu.org/viewcvs/gcc?view=revision&revision=212268 https://gcc.gnu.org/ml/gcc-patches/2014-07/msg00218.html - r219606 https://gcc.gnu.org/viewcvs/gcc?view=revision&revision=219606 https://gcc.gnu.org/ml/gcc-patches/2015-01/msg00615.html Should these also be back-ported to 4.9 branch (and in case of r219606, also follow-up fix r219722, https://gcc.gnu.org/viewcvs?rev=219722&root=gcc&view=rev , fix for https://gcc.gnu.org/bugzilla/show_bug.cgi?id=64605 )? Thanks, - Tom
On Tue, Feb 16, 2016 at 05:52:55PM +0100, Tom de Vries wrote: > Committed both patches to 4.9 and 5 branches. > > In order to run testsuite/libgomp.fortran/declare-simd-4.f90 with the 4.9 > branch build, I needed in addition: > - r212268 > https://gcc.gnu.org/viewcvs/gcc?view=revision&revision=212268 > https://gcc.gnu.org/ml/gcc-patches/2014-07/msg00218.html > - r219606 > https://gcc.gnu.org/viewcvs/gcc?view=revision&revision=219606 > https://gcc.gnu.org/ml/gcc-patches/2015-01/msg00615.html > > Should these also be back-ported to 4.9 branch (and in case of r219606, also > follow-up fix r219722, > https://gcc.gnu.org/viewcvs?rev=219722&root=gcc&view=rev , fix for > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=64605 )? IMHO just don't backport that test, test it by hand only. Jakub
On 16/02/16 17:54, Jakub Jelinek wrote: > On Tue, Feb 16, 2016 at 05:52:55PM +0100, Tom de Vries wrote: >> Committed both patches to 4.9 and 5 branches. >> >> In order to run testsuite/libgomp.fortran/declare-simd-4.f90 with the 4.9 >> branch build, I needed in addition: >> - r212268 >> https://gcc.gnu.org/viewcvs/gcc?view=revision&revision=212268 >> https://gcc.gnu.org/ml/gcc-patches/2014-07/msg00218.html >> - r219606 >> https://gcc.gnu.org/viewcvs/gcc?view=revision&revision=219606 >> https://gcc.gnu.org/ml/gcc-patches/2015-01/msg00615.html >> >> Should these also be back-ported to 4.9 branch (and in case of r219606, also >> follow-up fix r219722, >> https://gcc.gnu.org/viewcvs?rev=219722&root=gcc&view=rev , fix for >> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=64605 )? > > IMHO just don't backport that test, test it by hand only. > I've already committed the testcase, and without these two patches the test shows up as unsupported, so I'll leave it at that. As for the lto bit (r219606) I can see your point, it's the only libgomp test that uses lto. But there are 16 test cases which are currently unsupported in the 4.9 branch, and would be supported with the fortran patch (r212268): ... libgomp.fortran/allocatable11.f90 libgomp.fortran/allocatable2.f90 libgomp.fortran/allocatable8.f90 libgomp.fortran/appendix-a/a.22.7.f90 libgomp.fortran/appendix-a/a.22.8.f90 libgomp.fortran/crayptr2.f90 libgomp.fortran/declare-simd-1.f90 libgomp.fortran/declare-simd-2.f90 libgomp.fortran/omp_parse3.f90 libgomp.fortran/pointer2.f90 libgomp.fortran/pr25162.f libgomp.fortran/pr32550.f90 libgomp.fortran/threadprivate1.f90 libgomp.fortran/threadprivate2.f90 libgomp.fortran/threadprivate3.f90 libgomp.fortran/threadprivate4.f90 ... Perhaps that makes it worthwhile to back-port the patch. Thanks, - Tom
Don't call call_cgraph_insertion_hooks in simd_clone_create 2016-02-08 Tom de Vries <tom@codesourcery.com> PR lto/67709 * omp-low.c (simd_clone_create): Remove call to symtab->call_cgraph_insertion_hooks. * testsuite/libgomp.fortran/declare-simd-4.f90: New test. --- gcc/omp-low.c | 1 - libgomp/testsuite/libgomp.fortran/declare-simd-4.f90 | 7 +++++++ 2 files changed, 7 insertions(+), 1 deletion(-) diff --git a/gcc/omp-low.c b/gcc/omp-low.c index d41688b..fcbb3e0 100644 --- a/gcc/omp-low.c +++ b/gcc/omp-low.c @@ -18735,7 +18735,6 @@ simd_clone_create (struct cgraph_node *old_node) new_node = old_node->create_version_clone (new_decl, vNULL, NULL); if (old_node->in_other_partition) new_node->in_other_partition = 1; - symtab->call_cgraph_insertion_hooks (new_node); } if (new_node == NULL) return new_node; diff --git a/libgomp/testsuite/libgomp.fortran/declare-simd-4.f90 b/libgomp/testsuite/libgomp.fortran/declare-simd-4.f90 new file mode 100644 index 0000000..bfdf9cf --- /dev/null +++ b/libgomp/testsuite/libgomp.fortran/declare-simd-4.f90 @@ -0,0 +1,7 @@ +! { dg-do run { target { vect_simd_clones && lto } } } +! { dg-options "-fno-inline -flto -fno-use-linker-plugin" } +! { dg-additional-sources declare-simd-3.f90 } +! { dg-additional-options "-msse2" { target sse2_runtime } } +! { dg-additional-options "-mavx" { target avx_runtime } } + +include 'declare-simd-2.f90'
Hi, Consider libgomp.fortran/declare-simd-3.f90: ... subroutine bar use declare_simd_2_mod real :: b(128) integer :: i !$omp simd do i = 1, 128 b(i) = i * 2.0 end do !$omp simd do i = 1, 128 b(i) = foo (7.0_8, 5 * i, b(i)) end do do i = 1, 128 if (b(i).ne.(7.0 + 10.0 * i * i)) call abort end do end subroutine bar ... when compiling declare-simd-3.f90 with '-O0 -fopenmp -flto -fno-use-linker-plugin', we run into an ICE with backtrace: ... ICE backtrace: ... src/libgomp/testsuite/libgomp.fortran/declare-simd-3.f90: At top level: src/libgomp/testsuite/libgomp.fortran/declare-simd-3.f90:7:0: internal compiler error: in estimate_function_body_sizes, at ipa-inline-analysis.c:2486 use declare_simd_2_mod 0xc9319d estimate_function_body_sizes src/gcc/ipa-inline-analysis.c:2486 0xc950dd compute_inline_parameters(cgraph_node*, bool) src/gcc/ipa-inline-analysis.c:2953 0xc9813b inline_analyze_function(cgraph_node*) src/gcc/ipa-inline-analysis.c:4078 0xc98205 inline_summary_t::insert(cgraph_node*, inline_summary*) src/gcc/ipa-inline-analysis.c:4105 0x9a6213 symbol_table::call_cgraph_insertion_hooks(cgraph_node*) src/gcc/cgraph.c:371 0xdefa0e simd_clone_create src/gcc/omp-low.c:18738 0xdf5012 expand_simd_clones src/gcc/omp-low.c:19799 0xdf519b ipa_omp_simd_clone src/gcc/omp-low.c:19839 0xdf520a execute src/gcc/omp-low.c:19867 Please submit a full bug report, ... During pass_omp_simd_clone, we call simd_clone_create for foo, and execute the !old_node->definition part: ... tree old_decl = old_node->decl; tree new_decl = copy_node (old_node->decl); DECL_NAME (new_decl) = clone_function_name (old_decl, "simdclone"); SET_DECL_ASSEMBLER_NAME (new_decl, DECL_NAME (new_decl)); SET_DECL_RTL (new_decl, NULL); DECL_STATIC_CONSTRUCTOR (new_decl) = 0; DECL_STATIC_DESTRUCTOR (new_decl) = 0; new_node = old_node->create_version_clone (new_decl, vNULL, NULL); if (old_node->in_other_partition) new_node->in_other_partition = 1; symtab->call_cgraph_insertion_hooks (new_node); ... The 'symtab->call_cgraph_insertion_hooks (new_node)' calls 'inline_summary_t::insert', a hook inserted during pass_ipa_inline. During execution of the hook we stumble over the fact that the new node has no 'struct function' in estimate_function_body_sizes: ... struct function *my_function = DECL_STRUCT_FUNCTION (node->decl); ... gcc_assert (my_function && my_function->cfg); ... The patch fixes the ICE by removing the call to 'symtab->call_cgraph_insertion_hooks'. [ The pass before pass_omp_simd_clone is pass_dispatcher_calls. It has a function create_target_clone, similar to simd_clone_create, with a node.defition and !node.defition part. The !node.defition part does not call 'symtab->call_cgraph_insertion_hooks (new_node)'. ] Bootstrapped and reg-tested on x86_64. OK for stage1 trunk? Thanks, - Tom