[PATCH] Fix for PR ipa/64693
Martin Liška
mliska@suse.cz
Fri Feb 13 14:05:00 GMT 2015
On 02/13/2015 02:37 PM, Martin Liška wrote:
> On 02/12/2015 05:57 PM, Jan Hubicka wrote:
>>> From: mliska <mliska@suse.cz>V
>>> Date: Fri, 23 Jan 2015 14:58:36 +0100
>>> Subject: [PATCH] Fix PR ipa/64693.
>>>
>>> gcc/ChangeLog:
>>>
>>> 2015-02-12 Martin Liska <mliska@suse.cz>
>>>
>>> PR ipa/64693
>>> * ipa-icf.c (sem_item_optimizer::execute): Call identify_address_sensitive_classes.
>>> (sem_item_optimizer::identify_address_sensitive_classes): New function.
>>> (sem_item_optimizer::merge_classes): Handle cases where we merge a class
>>> with a sensitive reference.
>>> (congruence_class::dump): Add support for a new sensitive_reference flag.
>>> * ipa-icf.h: Add new function.
>>>
>>> +
>>> +/* Identify congruence classes which have an address taken. These
>>> + classes can be potentially been merged just as thunks. */
>>
>> Hmm, I would expect someting like this
>>
>> 1) break out code in sem_function::merge which decides if function A and B
>> can be identified or if thunk is needed
>> 2) when comparing references to verify congruence, actually check if merging
>> would result in thunk and subdivide.
>>
>> In the following:
>>
>> void f1()
>> {
>> }
>> void f2()
>> {
>> }
>> void *a=&f1;
>> void *b=&f1;
>> void *c=&f2;
>> void *d=&f2;
>>
>> The code bellow seems to disable all merging, while we ought to be able to mere A,B
>> and C,D into two congruences.
>> Similarly we ought to be able to merge in a case F2 is a virtual function/ctor/dtor
>> where address can not be compared for equality.
>
> Hello.
>
> The code below works fine with this example:
> Semantic equality hit:d->c
> Semantic equality hit:b->a
> Semantic equality hit:f2->f1
>
> It just provides guard for following very special situation (cc1 LTO contains just 2 classes with this example),
> which is in gcc/testsuite/gcc.dg/ipa/ipa-icf-34.c:
>
> Where we have functions in these classes:
> class 1:
> foo,bar
>
> class 2:
> callback1, callback2
>
> Where foo references callback1 and bar points to callback2. As callback{1,2} are in the same congruence class
> we can process merge operation of foo and bar. Unfortunately, address of callback{1,2} is taken and in this case
> we cannot merge foo and bar.
>
> As you said, to make things really correct, one should split class 1 according to these references.
> Question is if we want to do it for GCC 5.0 or prepare if for next release?
>
> I'm going to measure number of occurrences in Firefox and can make a decision.
> I like the idea of separating function in a class to candidates that need thunk creation and the rest. That will
> work seamlessly with your idea to include all functions that are just 'wrappers' for a function.
>
> Thanks for ideas,
> Martin
>
Hi.
As I expected, there are just 17 classes (mainly formed by pair of functions) that are marked
by the patch as reference_sensitive.
Martin
>>> +
>>> +void
>>> +sem_item_optimizer::identify_address_sensitive_classes (void)
>>> +{
>>> + for (hash_table<congruence_class_group_hash>::iterator it = m_classes.begin ();
>>> + it != m_classes.end (); ++it)
>>> + for (unsigned i = 0; i < (*it)->classes.length (); i++)
>>> + {
>>> + congruence_class *cls = (*it)->classes[i];
>>> +
>>> + if (cls->members.length () == 1)
>>> + continue;
>>> +
>>> + auto_vec <sem_item *> references;
>>> + sem_item *source_node = cls->members[0];
>>> + ipa_ref *r;
>>> +
>>> + for (unsigned j = 0; j < source_node->node->num_references (); j++)
>>> + {
>>> + symtab_node *ref = source_node->node->iterate_reference (j, r)->referred;
>>> + sem_item **symbol = m_symtab_node_map.get (ref);
>>> + if (symbol)
>>> + references.safe_push (*symbol);
>>> + }
>>> +
>>> + for (unsigned j = 1; j < cls->members.length (); j++)
>>> + {
>>> + source_node = cls->members[j];
>>> +
>>> + unsigned l = 0;
>>> + for (unsigned k = 0; k < source_node->node->num_references (); k++)
>>> + {
>>> + symtab_node *ref = source_node->node->iterate_reference (k, r)->referred;
>>> + sem_item **symbol = m_symtab_node_map.get (ref);
>>> + if (symbol)
>>> + {
>>> + if (l >= references.length () || references[l] != *symbol)
>>> + {
>>> + cls->sensitive_reference = true;
>>> + break;
>>> + }
>>> +
>>> + l++;
>>> + }
>>> + }
>>> +
>>> + if (cls->sensitive_reference)
>>> + break;
>>> + }
>>> + }
>>> +}
>>> +
>>> /* Debug function prints all informations about congruence classes. */
>>>
>>> void
>>> @@ -2374,6 +2430,15 @@ sem_item_optimizer::merge_classes (unsigned int prev_class_count)
>>> continue;
>>> }
>>>
>>> + if (c->sensitive_reference)
>>> + {
>>> + if (dump_file)
>>> + fprintf (dump_file, "A function from the congruence class "
>>> + "uses a sensitive reference.\n");
>>> +
>>> + continue;
>>> + }
>>> +
>>> if (dump_file && (dump_flags & TDF_DETAILS))
>>> {
>>> source->dump_to_file (dump_file);
>>> @@ -2390,8 +2455,10 @@ sem_item_optimizer::merge_classes (unsigned int prev_class_count)
>>> void
>>> congruence_class::dump (FILE *file, unsigned int indent) const
>>> {
>>> - FPRINTF_SPACES (file, indent, "class with id: %u, hash: %u, items: %u\n",
>>> - id, members[0]->get_hash (), members.length ());
>>> + FPRINTF_SPACES (file, indent,
>>> + "class with id: %u, hash: %u, items: %u %s\n",
>>> + id, members[0]->get_hash (), members.length (),
>>> + sensitive_reference ? "sensitive_reference" : "");
>>>
>>> FPUTS_SPACES (file, indent + 2, "");
>>> for (unsigned i = 0; i < members.length (); i++)
>>> diff --git a/gcc/ipa-icf.h b/gcc/ipa-icf.h
>>> index adbedd6..db0bc54 100644
>>> --- a/gcc/ipa-icf.h
>>> +++ b/gcc/ipa-icf.h
>>> @@ -29,7 +29,8 @@ class congruence_class
>>> {
>>> public:
>>> /* Congruence class constructor for a new class with _ID. */
>>> - congruence_class (unsigned int _id): in_worklist (false), id(_id)
>>> + congruence_class (unsigned int _id): in_worklist (false), id(_id),
>>> + sensitive_reference (false)
>>> {
>>> }
>>>
>>> @@ -54,6 +55,9 @@ public:
>>>
>>> /* Global unique class identifier. */
>>> unsigned int id;
>>> +
>>> + /* A function from the class contains a reference to another class. */
>>> + bool sensitive_reference;
>>> };
>>>
>>> /* Semantic item type enum. */
>>> @@ -476,6 +480,10 @@ private:
>>> /* Iterative congruence reduction function. */
>>> void process_cong_reduction (void);
>>>
>>> + /* Identify congruence classes which have an address taken. These
>>> + classes can be potentially been merged just as thunks. */
>>> + void identify_address_sensitive_classes (void);
>>> +
>>> /* After reduction is done, we can declare all items in a group
>>> to be equal. PREV_CLASS_COUNT is start number of classes
>>> before reduction. */
>>> diff --git a/gcc/testsuite/gcc.dg/ipa/ipa-icf-26.c b/gcc/testsuite/gcc.dg/ipa/ipa-icf-26.c
>>> index 0c5a5a6..f25a251 100644
>>> --- a/gcc/testsuite/gcc.dg/ipa/ipa-icf-26.c
>>> +++ b/gcc/testsuite/gcc.dg/ipa/ipa-icf-26.c
>>> @@ -38,7 +38,7 @@ int main()
>>> return 0;
>>> }
>>>
>>> -/* { dg-final { scan-ipa-dump "Semantic equality hit:bar->foo" "icf" } } */
>>> /* { dg-final { scan-ipa-dump "Semantic equality hit:remove->destroy" "icf" } } */
>>> /* { dg-final { scan-ipa-dump "Equal symbols: 2" "icf" } } */
>>> +/* { dg-final { scan-ipa-dump "A function from the congruence class uses a sensitive reference." "icf" } } */
>>> /* { dg-final { cleanup-ipa-dump "icf" } } */
>>> diff --git a/gcc/testsuite/gcc.dg/ipa/ipa-icf-33.c b/gcc/testsuite/gcc.dg/ipa/ipa-icf-33.c
>>> index d7e814d..7b6a8ae 100644
>>> --- a/gcc/testsuite/gcc.dg/ipa/ipa-icf-33.c
>>> +++ b/gcc/testsuite/gcc.dg/ipa/ipa-icf-33.c
>>> @@ -23,5 +23,4 @@ int main()
>>> }
>>>
>>> /* { dg-final { scan-ipa-dump "Equal symbols: 1" "icf" } } */
>>> -/* { dg-final { scan-ipa-dump "Equal symbols: 1" "icf" } } */
>>> /* { dg-final { cleanup-ipa-dump "icf" } } */
>>> diff --git a/gcc/testsuite/gcc.dg/ipa/ipa-icf-34.c b/gcc/testsuite/gcc.dg/ipa/ipa-icf-34.c
>>> new file mode 100644
>>> index 0000000..66bcac5
>>> --- /dev/null
>>> +++ b/gcc/testsuite/gcc.dg/ipa/ipa-icf-34.c
>>> @@ -0,0 +1,44 @@
>>> +/* { dg-do compile } */
>>> +/* { dg-options "-O0 -fipa-icf -fdump-ipa-icf-details" } */
>>> +
>>> +#include <stdlib.h>
>>> +#include <assert.h>
>>> +
>>> +int callback1(int a)
>>> +{
>>> + return a * a;
>>> +}
>>> +
>>> +int callback2(int a)
>>> +{
>>> + return a * a;
>>> +}
>>> +
>>> +static int test(int (*callback) (int))
>>> +{
>>> + if (callback == callback1)
>>> + return 1;
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +int foo()
>>> +{
>>> + return test(&callback1);
>>> +}
>>> +
>>> +int bar()
>>> +{
>>> + return test(&callback2);
>>> +}
>>> +
>>> +int main()
>>> +{
>>> + assert (foo() != bar());
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +/* { dg-final { scan-ipa-dump "Equal symbols: 2" "icf" } } */
>>> +/* { dg-final { scan-ipa-dump "A function from the congruence class uses a sensitive reference." "icf" } } */
>>> +/* { dg-final { cleanup-ipa-dump "icf" } } */
>>> --
>>> 2.1.2
>>>
>>
>
More information about the Gcc-patches
mailing list