On Fri, Oct 26, 2012 at 9:07 AM, Xinliang David Li <davi...@google.com> wrote:
> On Fri, Oct 26, 2012 at 8:54 AM, Jan Hubicka <hubi...@ucw.cz> wrote:
>> Hi,
>> sorry for jumping in late, for too long I did not had chnce to look at my
>> TODO.
>> I have two comments...
>>> Index: gcc/cgraphbuild.c
>>> ===================================================================
>>> --- gcc/cgraphbuild.c (revision 192623)
>>> +++ gcc/cgraphbuild.c (working copy)
>>> @@ -34,6 +34,7 @@ along with GCC; see the file COPYING3. If not see
>>> #include "ipa-utils.h"
>>> #include "except.h"
>>> #include "ipa-inline.h"
>>> +#include "target.h"
>>>
>>> /* Context of record_reference. */
>>> struct record_reference_ctx
>>> @@ -317,8 +318,23 @@ build_cgraph_edges (void)
>>> bb);
>>> decl = gimple_call_fndecl (stmt);
>>> if (decl)
>>> - cgraph_create_edge (node, cgraph_get_create_node (decl),
>>> - stmt, bb->count, freq);
>>> + {
>>> + struct cgraph_node *callee = cgraph_get_create_node (decl);
>>> + /* If a call to a multiversioned function dispatcher is
>>> + found, generate the body to dispatch the right function
>>> + at run-time. */
>>> + if (callee->dispatcher_function)
>>> + {
>>> + tree resolver_decl;
>>> + gcc_assert (callee->function_version
>>> + && callee->function_version->next);
>>> + gcc_assert (targetm.generate_version_dispatcher_body);
>>> + resolver_decl
>>> + = targetm.generate_version_dispatcher_body (callee);
>>> + gcc_assert (resolver_decl != NULL_TREE);
>>> + }
>>> + cgraph_create_edge (node, callee, stmt, bb->count, freq);
>>> + }
>> I do not really think resolver generation belongs here + I would preffer
>> build_cgraph_edges to really just build the edges.
>>> Index: gcc/cgraph.c
>>> ===================================================================
>>> --- gcc/cgraph.c (revision 192623)
>>> +++ gcc/cgraph.c (working copy)
>>> @@ -1277,6 +1277,16 @@ cgraph_mark_address_taken_node (struct cgraph_node
>>> node->symbol.address_taken = 1;
>>> node = cgraph_function_or_thunk_node (node, NULL);
>>> node->symbol.address_taken = 1;
>>> + /* If the address of a multiversioned function dispatcher is taken,
>>> + generate the body to dispatch the right function at run-time. This
>>> + is needed as the address can be used to do an indirect call. */
>>> + if (node->dispatcher_function)
>>> + {
>>> + gcc_assert (node->function_version
>>> + && node->function_version->next);
>>> + gcc_assert (targetm.generate_version_dispatcher_body);
>>> + targetm.generate_version_dispatcher_body (node);
>>> + }
>>
>> Similarly here. I also think this way you will miss aliases of the
>> multiversioned
>> functions.
>
>>
>> I am not sure why the multiversioning is tied with the cgraph build and the
>> datastructure is put into cgraph_node itself. It seems to me that your
>> dispatchers are in a way related to thunks - i.e. they are inserted into
>> callgraph and once they become reachable their body needs to be produced. I
>> think generate_version_dispatcher_body should thus probably be done from
>> cgraph_analyze_function. (to make the function to be seen by analyze_function
>> you will need to make it to be finalized at the time you set
>> dispatcher_function flag.
>
> This seems reasonable -- Sri, do you see any problems with this suggestion?
No, I will make this change asap.
>
>>
>> I would also put the dispatcher datastructure into on-side hash by node->uid.
>> (i.e. these are rare and thus the datastructure should be small)
>> symbol table is critical for WPA stage memory use and I plan to remove as
>> much
>> as possible from the nodes in near future. For this reason I would preffer
>> to not add too much of stuff that is not going to be used by majority of
>> nodes.
>>
OK, will change as suggested.
>
> I had the concern on the increasing the size of core data structure too.
Thanks,
-Sri.
>
> thanks,
>
> David
>
>> Honza
