| Message ID | 20140414173646.GE8020@virgil.suse |
|---|---|
| State | New |
| Headers | show |
> Hi, > > I have noticed that we often run IPA-transformation phases multiple > times. The reason is that when a virtual clone is created, it starts > with an empty ipa_transforms_to_apply vector and all subsequent IPA > passes that see it are pushed onto it. The original node gets these > pushed on their ipa_transforms_to_apply too. Later on, when a clone > receives its body, in tree_function_versioning, the contents of > original node's ipa_transforms_to_apply is added to clone's > ipa_transforms_to_apply (in a slightly convoluted way). I do not > understand how this was supposed to work but obviously we can and do > end up with multiple copies of a few passes in ipa_transforms_to_apply > of the clone. > > I believe the correct thing to do is to copy the vector when the > virtual clone is created and remove the part in > tree_function_versioning that does the copying. I have verified that > we no longer call transformation of IPA-CP multiple times when we > previously did and added asserts to check that no other caller of > tree_function_versioning expects to have the vector copied. > > Bootstrapped and tested on x86_64-linux, OK for trunk? OK, I believe this is artifact of times we had transforms in DECL_STRUCT_FUNCTION. Honza > > Thanks, > > Martin > > > 2014-04-10 Martin Jambor <mjambor@suse.cz> > > * cgraphclones.c (cgraph_create_virtual_clone): Duplicate > ipa_transforms_to_apply. > (cgraph_function_versioning): Assert that old_node has empty > ipa_transforms_to_apply. > * trans-mem.c (ipa_tm_create_version): Likewise. > * tree-inline.c (tree_function_versioning): Do not duplicate > ipa_transforms_to_apply. > > Index: src/gcc/cgraphclones.c > =================================================================== > --- src.orig/gcc/cgraphclones.c > +++ src/gcc/cgraphclones.c > @@ -600,6 +600,9 @@ cgraph_create_virtual_clone (struct cgra > } > else > new_node->clone.combined_args_to_skip = args_to_skip; > + if (old_node->ipa_transforms_to_apply.exists ()) > + new_node->ipa_transforms_to_apply > + = old_node->ipa_transforms_to_apply.copy (); > > cgraph_call_node_duplication_hooks (old_node, new_node); > > @@ -971,6 +974,7 @@ cgraph_function_versioning (struct cgrap > cgraph_copy_node_for_versioning (old_version_node, new_decl, > redirect_callers, bbs_to_copy); > > + gcc_assert (!old_version_node->ipa_transforms_to_apply.exists ()); > /* Copy the OLD_VERSION_NODE function tree to the new version. */ > tree_function_versioning (old_decl, new_decl, tree_map, false, args_to_skip, > skip_return, bbs_to_copy, new_entry_block); > Index: src/gcc/trans-mem.c > =================================================================== > --- src.orig/gcc/trans-mem.c > +++ src/gcc/trans-mem.c > @@ -4914,6 +4914,7 @@ ipa_tm_create_version (struct cgraph_nod > if (DECL_ONE_ONLY (new_decl)) > DECL_COMDAT_GROUP (new_decl) = tm_mangle (DECL_COMDAT_GROUP (old_decl)); > > + gcc_assert (!old_node->ipa_transforms_to_apply.exists ()); > new_node = cgraph_copy_node_for_versioning (old_node, new_decl, vNULL, NULL); > new_node->local.local = false; > new_node->externally_visible = old_node->externally_visible; > Index: src/gcc/tree-inline.c > =================================================================== > --- src.orig/gcc/tree-inline.c > +++ src/gcc/tree-inline.c > @@ -5323,18 +5323,6 @@ tree_function_versioning (tree old_decl, > id.dst_node = new_version_node; > id.src_cfun = DECL_STRUCT_FUNCTION (old_decl); > id.blocks_to_copy = blocks_to_copy; > - if (id.src_node->ipa_transforms_to_apply.exists ()) > - { > - vec<ipa_opt_pass> old_transforms_to_apply > - = id.dst_node->ipa_transforms_to_apply; > - unsigned int i; > - > - id.dst_node->ipa_transforms_to_apply > - = id.src_node->ipa_transforms_to_apply.copy (); > - for (i = 0; i < old_transforms_to_apply.length (); i++) > - id.dst_node->ipa_transforms_to_apply.safe_push (old_transforms_to_apply[i]); > - old_transforms_to_apply.release (); > - } > > id.copy_decl = copy_decl_no_change; > id.transform_call_graph_edges
Likely after this was checked in appeared following on x86 FAIL: gcc.dg/vect/vect-simd-clone-11.c -flto -ffat-lto-objects (internal compiler error) FAIL: gcc.dg/vect/vect-simd-clone-11.c -flto -ffat-lto-objects (test for excess errors) FAIL: gcc.dg/vect/vect-simd-clone-12.c -flto -ffat-lto-objects (internal compiler error) FAIL: gcc.dg/vect/vect-simd-clone-12.c -flto -ffat-lto-objects (test for excess errors) FAIL: gcc.dg/vect/vect-simd-clone-1.c -flto -ffat-lto-objects (internal compiler error) FAIL: gcc.dg/vect/vect-simd-clone-1.c -flto -ffat-lto-objects (test for excess errors) FAIL: gcc.dg/vect/vect-simd-clone-2.c -flto -ffat-lto-objects (internal compiler error) FAIL: gcc.dg/vect/vect-simd-clone-2.c -flto -ffat-lto-objects (test for excess errors) FAIL: gcc.dg/vect/vect-simd-clone-3.c -flto -ffat-lto-objects (internal compiler error) FAIL: gcc.dg/vect/vect-simd-clone-3.c -flto -ffat-lto-objects (test for excess errors) FAIL: gcc.dg/vect/vect-simd-clone-4.c -flto -ffat-lto-objects (internal compiler error) FAIL: gcc.dg/vect/vect-simd-clone-4.c -flto -ffat-lto-objects (test for excess errors) FAIL: gcc.dg/vect/vect-simd-clone-5.c -flto -ffat-lto-objects (internal compiler error) FAIL: gcc.dg/vect/vect-simd-clone-5.c -flto -ffat-lto-objects (test for excess errors) FAIL: gcc.dg/vect/vect-simd-clone-6.c -flto -ffat-lto-objects (internal compiler error) FAIL: gcc.dg/vect/vect-simd-clone-6.c -flto -ffat-lto-objects (test for excess errors) FAIL: gcc.dg/vect/vect-simd-clone-7.c -flto -ffat-lto-objects (internal compiler error) FAIL: gcc.dg/vect/vect-simd-clone-7.c -flto -ffat-lto-objects (test for excess errors) FAIL: gcc.dg/vect/vect-simd-clone-8.c -flto -ffat-lto-objects (internal compiler error) FAIL: gcc.dg/vect/vect-simd-clone-8.c -flto -ffat-lto-objects (test for excess errors) FAIL: gcc.dg/vect/vect-simd-clone-9.c -flto -ffat-lto-objects (internal compiler error) FAIL: gcc.dg/vect/vect-simd-clone-9.c -flto -ffat-lto-objects (test for excess errors) > -----Original Message----- > From: gcc-patches-owner@gcc.gnu.org [mailto:gcc-patches- > owner@gcc.gnu.org] On Behalf Of Martin Jambor > Sent: Monday, April 14, 2014 9:37 PM > To: GCC Patches > Cc: Jan Hubicka > Subject: [PATCH] Do not run IPA transform phases multiple times > > Hi, > > I have noticed that we often run IPA-transformation phases multiple times. > The reason is that when a virtual clone is created, it starts with an empty > ipa_transforms_to_apply vector and all subsequent IPA passes that see it are > pushed onto it. The original node gets these pushed on their > ipa_transforms_to_apply too. Later on, when a clone receives its body, in > tree_function_versioning, the contents of original node's > ipa_transforms_to_apply is added to clone's ipa_transforms_to_apply (in a > slightly convoluted way). I do not understand how this was supposed to > work but obviously we can and do end up with multiple copies of a few > passes in ipa_transforms_to_apply of the clone. > > I believe the correct thing to do is to copy the vector when the virtual clone is > created and remove the part in tree_function_versioning that does the > copying. I have verified that we no longer call transformation of IPA-CP > multiple times when we previously did and added asserts to check that no > other caller of tree_function_versioning expects to have the vector copied. > > Bootstrapped and tested on x86_64-linux, OK for trunk? > > Thanks, > > Martin > > > 2014-04-10 Martin Jambor <mjambor@suse.cz> > > * cgraphclones.c (cgraph_create_virtual_clone): Duplicate > ipa_transforms_to_apply. > (cgraph_function_versioning): Assert that old_node has empty > ipa_transforms_to_apply. > * trans-mem.c (ipa_tm_create_version): Likewise. > * tree-inline.c (tree_function_versioning): Do not duplicate > ipa_transforms_to_apply. > > Index: src/gcc/cgraphclones.c > ========================================================== > ========= > --- src.orig/gcc/cgraphclones.c > +++ src/gcc/cgraphclones.c > @@ -600,6 +600,9 @@ cgraph_create_virtual_clone (struct cgra > } > else > new_node->clone.combined_args_to_skip = args_to_skip; > + if (old_node->ipa_transforms_to_apply.exists ()) > + new_node->ipa_transforms_to_apply > + = old_node->ipa_transforms_to_apply.copy (); > > cgraph_call_node_duplication_hooks (old_node, new_node); > > @@ -971,6 +974,7 @@ cgraph_function_versioning (struct cgrap > cgraph_copy_node_for_versioning (old_version_node, new_decl, > redirect_callers, bbs_to_copy); > > + gcc_assert (!old_version_node->ipa_transforms_to_apply.exists ()); > /* Copy the OLD_VERSION_NODE function tree to the new version. */ > tree_function_versioning (old_decl, new_decl, tree_map, false, > args_to_skip, > skip_return, bbs_to_copy, new_entry_block); > Index: src/gcc/trans-mem.c > ========================================================== > ========= > --- src.orig/gcc/trans-mem.c > +++ src/gcc/trans-mem.c > @@ -4914,6 +4914,7 @@ ipa_tm_create_version (struct cgraph_nod > if (DECL_ONE_ONLY (new_decl)) > DECL_COMDAT_GROUP (new_decl) = tm_mangle > (DECL_COMDAT_GROUP (old_decl)); > > + gcc_assert (!old_node->ipa_transforms_to_apply.exists ()); > new_node = cgraph_copy_node_for_versioning (old_node, new_decl, > vNULL, NULL); > new_node->local.local = false; > new_node->externally_visible = old_node->externally_visible; > Index: src/gcc/tree-inline.c > ========================================================== > ========= > --- src.orig/gcc/tree-inline.c > +++ src/gcc/tree-inline.c > @@ -5323,18 +5323,6 @@ tree_function_versioning (tree old_decl, > id.dst_node = new_version_node; > id.src_cfun = DECL_STRUCT_FUNCTION (old_decl); > id.blocks_to_copy = blocks_to_copy; > - if (id.src_node->ipa_transforms_to_apply.exists ()) > - { > - vec<ipa_opt_pass> old_transforms_to_apply > - = id.dst_node->ipa_transforms_to_apply; > - unsigned int i; > - > - id.dst_node->ipa_transforms_to_apply > - = id.src_node->ipa_transforms_to_apply.copy (); > - for (i = 0; i < old_transforms_to_apply.length (); i++) > - id.dst_node->ipa_transforms_to_apply.safe_push > (old_transforms_to_apply[i]); > - old_transforms_to_apply.release (); > - } > > id.copy_decl = copy_decl_no_change; > id.transform_call_graph_edges
On Wed, Apr 16, 2014 at 03:28:59PM +0000, Zamyatin, Igor wrote: > Likely after this was checked in appeared following on x86 > > FAIL: gcc.dg/vect/vect-simd-clone-11.c -flto -ffat-lto-objects (internal compiler error) > FAIL: gcc.dg/vect/vect-simd-clone-11.c -flto -ffat-lto-objects (test for excess errors) > FAIL: gcc.dg/vect/vect-simd-clone-12.c -flto -ffat-lto-objects (internal compiler error) > FAIL: gcc.dg/vect/vect-simd-clone-12.c -flto -ffat-lto-objects (test for excess errors) > FAIL: gcc.dg/vect/vect-simd-clone-1.c -flto -ffat-lto-objects (internal compiler error) > FAIL: gcc.dg/vect/vect-simd-clone-1.c -flto -ffat-lto-objects (test for excess errors) > FAIL: gcc.dg/vect/vect-simd-clone-2.c -flto -ffat-lto-objects (internal compiler error) > FAIL: gcc.dg/vect/vect-simd-clone-2.c -flto -ffat-lto-objects (test for excess errors) > FAIL: gcc.dg/vect/vect-simd-clone-3.c -flto -ffat-lto-objects (internal compiler error) > FAIL: gcc.dg/vect/vect-simd-clone-3.c -flto -ffat-lto-objects (test for excess errors) > FAIL: gcc.dg/vect/vect-simd-clone-4.c -flto -ffat-lto-objects (internal compiler error) > FAIL: gcc.dg/vect/vect-simd-clone-4.c -flto -ffat-lto-objects (test for excess errors) > FAIL: gcc.dg/vect/vect-simd-clone-5.c -flto -ffat-lto-objects (internal compiler error) > FAIL: gcc.dg/vect/vect-simd-clone-5.c -flto -ffat-lto-objects (test for excess errors) > FAIL: gcc.dg/vect/vect-simd-clone-6.c -flto -ffat-lto-objects (internal compiler error) > FAIL: gcc.dg/vect/vect-simd-clone-6.c -flto -ffat-lto-objects (test for excess errors) > FAIL: gcc.dg/vect/vect-simd-clone-7.c -flto -ffat-lto-objects (internal compiler error) > FAIL: gcc.dg/vect/vect-simd-clone-7.c -flto -ffat-lto-objects (test for excess errors) > FAIL: gcc.dg/vect/vect-simd-clone-8.c -flto -ffat-lto-objects (internal compiler error) > FAIL: gcc.dg/vect/vect-simd-clone-8.c -flto -ffat-lto-objects (test for excess errors) > FAIL: gcc.dg/vect/vect-simd-clone-9.c -flto -ffat-lto-objects (internal compiler error) > FAIL: gcc.dg/vect/vect-simd-clone-9.c -flto -ffat-lto-objects (test for excess errors) Yeah, it is in the assert added in this patch: 977 gcc_assert (!old_version_node->ipa_transforms_to_apply.exists ()); in cgraph_function_versioning. pass_omp_simd_clone is a late IPA pass which needs to perform cgraph_function_versioning, and the ICE is in lto1 when the old_version_node has been read from the LTO IL from the object file, and ipa_transforms_to_apply contains tons of various transforms, but I suppose that during late IPA passes they are no longer performed. Martin, can you please fix this up? Jakub
Index: src/gcc/cgraphclones.c =================================================================== --- src.orig/gcc/cgraphclones.c +++ src/gcc/cgraphclones.c @@ -600,6 +600,9 @@ cgraph_create_virtual_clone (struct cgra } else new_node->clone.combined_args_to_skip = args_to_skip; + if (old_node->ipa_transforms_to_apply.exists ()) + new_node->ipa_transforms_to_apply + = old_node->ipa_transforms_to_apply.copy (); cgraph_call_node_duplication_hooks (old_node, new_node); @@ -971,6 +974,7 @@ cgraph_function_versioning (struct cgrap cgraph_copy_node_for_versioning (old_version_node, new_decl, redirect_callers, bbs_to_copy); + gcc_assert (!old_version_node->ipa_transforms_to_apply.exists ()); /* Copy the OLD_VERSION_NODE function tree to the new version. */ tree_function_versioning (old_decl, new_decl, tree_map, false, args_to_skip, skip_return, bbs_to_copy, new_entry_block); Index: src/gcc/trans-mem.c =================================================================== --- src.orig/gcc/trans-mem.c +++ src/gcc/trans-mem.c @@ -4914,6 +4914,7 @@ ipa_tm_create_version (struct cgraph_nod if (DECL_ONE_ONLY (new_decl)) DECL_COMDAT_GROUP (new_decl) = tm_mangle (DECL_COMDAT_GROUP (old_decl)); + gcc_assert (!old_node->ipa_transforms_to_apply.exists ()); new_node = cgraph_copy_node_for_versioning (old_node, new_decl, vNULL, NULL); new_node->local.local = false; new_node->externally_visible = old_node->externally_visible; Index: src/gcc/tree-inline.c =================================================================== --- src.orig/gcc/tree-inline.c +++ src/gcc/tree-inline.c @@ -5323,18 +5323,6 @@ tree_function_versioning (tree old_decl, id.dst_node = new_version_node; id.src_cfun = DECL_STRUCT_FUNCTION (old_decl); id.blocks_to_copy = blocks_to_copy; - if (id.src_node->ipa_transforms_to_apply.exists ()) - { - vec<ipa_opt_pass> old_transforms_to_apply - = id.dst_node->ipa_transforms_to_apply; - unsigned int i; - - id.dst_node->ipa_transforms_to_apply - = id.src_node->ipa_transforms_to_apply.copy (); - for (i = 0; i < old_transforms_to_apply.length (); i++) - id.dst_node->ipa_transforms_to_apply.safe_push (old_transforms_to_apply[i]); - old_transforms_to_apply.release (); - } id.copy_decl = copy_decl_no_change; id.transform_call_graph_edges