This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
Re: [PATCH] Fix for PR ipa/64693
- From: Jan Hubicka <hubicka at ucw dot cz>
- To: Martin Liška <mliska at suse dot cz>
- Cc: gcc-patches at gcc dot gnu dot org, "hubicka@ >> Jan Hubicka" <hubicka at ucw dot cz>
- Date: Wed, 25 Feb 2015 18:00:43 +0100
- Subject: Re: [PATCH] Fix for PR ipa/64693
- Authentication-results: sourceware.org; auth=none
- References: <54DC77A5 dot 4020409 at suse dot cz> <20150212165758 dot GA3301 at kam dot mff dot cuni dot cz> <54E7308B dot 3020400 at suse dot cz> <20150220183956 dot GD21632 at kam dot mff dot cuni dot cz> <54EDF616 dot 9070908 at suse dot cz>
> Hello Honza.
>
> I've updated the patch so that your notes are resolved. Moreover, I've added comparison
> for interposable symbols that are either target of reference or are called by a function.
> Please read the patch to verify the comparison is as you expected.
>
> I'm going to run testsuite.
>
> Thanks,
> Martin
> >From 8dae064e67e30537486e0d502fc5df39d37cee3e Mon Sep 17 00:00:00 2001
> From: mliska <mliska@suse.cz>
> Date: Thu, 19 Feb 2015 16:08:09 +0100
> Subject: [PATCH 1/3] Fix PR ipa/64693
>
> gcc/ChangeLog:
>
> 2015-02-20 Martin Liska <mliska@suse.cz>
>
> PR ipa/64693
> * ipa-icf.c (sem_item_optimizer::add_item_to_class): Identify if
> a newly added item has an address reference.
> (sem_item_optimizer::subdivide_classes_by_addr_references):
> New function.
> (sem_item_optimizer::process_cong_reduction): Include subdivision
> based on address references.
> * ipa-icf.h (struct addr_refs_hashmap_traits): New struct.
> (sem_item::is_nonvirtual_or_cdtor): New function.
>
> gcc/testsuite/ChangeLog:
>
> 2015-02-20 Martin Liska <mliska@suse.cz>
>
> * gcc.dg/ipa/ipa-icf-26.c: Update expected test results.
> * gcc.dg/ipa/ipa-icf-33.c: Remove duplicate line.
> * gcc.dg/ipa/ipa-icf-34.c: New test.
> diff --git a/gcc/ipa-icf.c b/gcc/ipa-icf.c
> index 494fdcf..fbb641d 100644
> --- a/gcc/ipa-icf.c
> +++ b/gcc/ipa-icf.c
> @@ -126,6 +126,40 @@ along with GCC; see the file COPYING3. If not see
> using namespace ipa_icf_gimple;
>
> namespace ipa_icf {
> +
> +/* Constructor. */
> +
> +addr_refs_collection::addr_refs_collection (symtab_node *node)
I gues because you now track two thinks, address references and interposable
symbols, perhaps the function name can reflect it.
Perhaps symbol_compare_collection sounds more precise, but I leave decision
on you.
> +{
> + m_references.create (0);
> + m_interposables.create (0);
> +
> + ipa_ref *ref;
> +
> + if (is_a <varpool_node *> (node) && DECL_VIRTUAL_P (node->decl))
> + return;
> +
> + for (unsigned i = 0; i < node->num_references (); i++)
> + {
> + ref = node->iterate_reference (i, ref);
> + if (ref->use == IPA_REF_ADDR
> + && sem_item::is_nonvirtual_or_cdtor (ref->referred->decl))
> + m_references.safe_push (ref->referred);
Since I introduced the address_matters predicate, just make
is_nonvirtual_or_cdtor a address_matters_p predicate of ipa_ref itself.
Test that reference is ADDR, referring is not virtual table and referred is is
non-virtual noncdotr.
It is better to have this centralized in symbol table predicates because later
we may want to get smarter.
> @@ -638,11 +672,11 @@ sem_function::merge (sem_item *alias_item)
>
> /* See if original and/or alias address can be compared for equality. */
> original_address_matters
> - = (!DECL_VIRTUAL_P (original->decl)
> + = (sem_item::is_nonvirtual_or_cdtor (original->decl)
> && (original->externally_visible
> || original->address_taken_from_non_vtable_p ()));
> alias_address_matters
> - = (!DECL_VIRTUAL_P (alias->decl)
> + = (sem_item::is_nonvirtual_or_cdtor (alias->decl)
> && (alias->externally_visible
> || alias->address_taken_from_non_vtable_p ()));
>
Lets levae this for incremental patch for the ::nerge revamp.
> @@ -1969,6 +2003,82 @@ sem_item_optimizer::subdivide_classes_by_equality (bool in_wpa)
> verify_classes ();
> }
>
> +/* Subdivide classes by address references that members of the class
> + reference. Example can be a pair of functions that have an address
> + taken from a function. If these addresses are different the class
> + is split. */
> +
> +unsigned
> +sem_item_optimizer::subdivide_classes_by_addr_references ()
Simialrly this needs update of name.
> @@ -2258,8 +2368,20 @@ sem_item_optimizer::process_cong_reduction (void)
> fprintf (dump_file, "Congruence class reduction\n");
>
> congruence_class *cls;
> - while ((cls = worklist_pop ()) != NULL)
> - do_congruence_step (cls);
> +
> + while(!worklist_empty ())
> + {
> + /* Process complete congruence reduction. */
> + while ((cls = worklist_pop ()) != NULL)
> + do_congruence_step (cls);
> +
> + /* Subdivide newly created classes according to references. */
> + unsigned new_classes = subdivide_classes_by_addr_references ();
Still do not see why this needs to be iterated within the loop and not just executed once ;)
> +class addr_refs_collection
> +{
> +public:
> + /* Constructor. */
> + addr_refs_collection (symtab_node *node);
> +
> + /* Destructor. */
> + ~addr_refs_collection ()
> + {
> + m_references.release ();
> + m_interposables.release ();
> + }
> +
> + /* Vector of address references. */
> + vec<symtab_node *> m_references;
> +
> + /* Vector of interposable references. */
> + vec<symtab_node *> m_interposables;
> +};
> + static bool
> + equal_keys (const addr_refs_collection *a,
> + const addr_refs_collection *b)
> + {
> + if (a->m_references.length () != b->m_references.length ())
> + return false;
> +
> + for (unsigned i = 0; i < a->m_references.length (); i++)
> + if (a->m_references[i]->equal_address_to (b->m_references[i]) != 1)
> + return false;
> +
> + for (unsigned i = 0; i < a->m_interposables.length (); i++)
> + if (!a->m_interposables[i]->semantically_equivalent_p
> + (b->m_interposables[i]))
> + return false;
Aha, only here I noticed you make difference between references and interposables.
ADDR_EXPR of interposable symbol should go to references, too.
I think it does not make difference whether you use equal_address_to
or semantically_equivalent_p in your setup with current logic in the preidcates,
but lets keep it this way; it is more robust.
OK with these changes.
Honza