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