Re: User directed Function Multiversioning via Function Overloading (issue5752064)

[Xinliang David Li <davidxl@google.com>]

On Fri, Oct 26, 2012 at 8:54 AM, Jan Hubicka <hubicka@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?

>
> 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.
>

I had the concern on the increasing the size of core data structure too.

thanks,

David

> Honza