Commit Message

Hi,
I have now committed the attached patch.
Thanks,
-Sri.
On Thu, Nov 1, 2012 at 7:53 PM, Sriraman Tallam <tmsriram@google.com> wrote:
> Hi Jason,>> I have made all the changes you mentioned and attached the new> patch. Summary of the important things changed:>> * The versions are collected at declaration time itself now.> * extern "C" functions are disallowed from being versions for now.> extern "C" functions have to be handled exactly like how the C> front-end would handle versioned functions. I will do this when I get> to the C front-end.> * Finalizing cgraph nodes is removed from front-end code.>>> Thanks,> -Sri.>>> On Wed, Oct 31, 2012 at 7:02 AM, Jason Merrill <jason@redhat.com> wrote:>> On 10/30/2012 05:49 PM, Sriraman Tallam wrote:>>>>>> AFAIU, this should not be a problem. For duplicate declarations,>>> duplicate_decls should merge them and they should never be seen here.>>> Did I miss something?>>>>>> With extern "C" functions you can have multiple declarations of the same>> function in different namespaces that are not duplicates, but still match.>> And I can't think what that test is supposed to be catching, anyway.>>>>>>> No, I thought about this but I did not want to handle this case in>>> this iteration. The dispatcher is created only once and if more>>> functions are declared later, they will not be dispatched atleast in>>> this iteration.>>>>>> I still think that instead of collecting the set of functions in overload>> resolution, they should be collected at declaration time and added to a>> vector in the cgraph information for use when generating the body of the>> dispatcher.>>>>>>> You talked about doing the dispatcher>>> building later, but I did it here since I am doing it only once.>>>>>> I still don't think this is the right place for it.>>>>>>> dispatcher_node does not have a body until it is generated in>>> cgraphunit.c, so cgraph does not mark this field before this is>>> processed in cgraph_analyze_function.>>>>>> That seems like something to address in your cgraph changes.>>>> Jason>>
Function Multiversioning

Comments

On 11/05/2012 09:38 PM, Sriraman Tallam wrote:
> + /* For multi-versioned functions, more than one match is just fine.> + Call decls_match to make sure they are different because they are> + versioned. */> + if (DECL_FUNCTION_VERSIONED (fn))> + {> + for (match = TREE_CHAIN (matches); match; match = TREE_CHAIN (match))> + if (!DECL_FUNCTION_VERSIONED (TREE_PURPOSE (match))> + || decls_match (fn, TREE_PURPOSE (match)))> + break;> + }
I still don't understand what this code is supposed to be doing. Please
remove it and instead modify the other loop to allow mismatches that are
versions of the same function.
> + /* If the olddecl is a version, so is the newdecl. */> + if (TREE_CODE (newdecl) == FUNCTION_DECL> + && DECL_FUNCTION_VERSIONED (olddecl))> + {> + DECL_FUNCTION_VERSIONED (newdecl) = 1;> + /* newdecl will be purged and is no longer a version. */> + delete_function_version (newdecl);> + }
Please make the comment clearer that the reason we're setting the flag
on the newdecl is so that it'll be copied back into the olddecl;
otherwise it seems odd to say it's a version and then it isn't a version.
> + /* If a pointer to a function that is multi-versioned is requested, the> + pointer to the dispatcher function is returned instead. This works> + well because indirectly calling the function will dispatch the right> + function version at run-time. */> + if (DECL_FUNCTION_VERSIONED (fn))> + {> + tree dispatcher_decl = NULL;> + gcc_assert (targetm.get_function_versions_dispatcher);> + dispatcher_decl = targetm.get_function_versions_dispatcher (fn);> + if (!dispatcher_decl)> + {> + error_at (input_location, "Pointer to a multiversioned function"> + " without a default is not allowed");> + return error_mark_node;> + }> + retrofit_lang_decl (dispatcher_decl);> + fn = dispatcher_decl;
This code should use the get_function_version_dispatcher function in
cp/call.c.
Jason

On Tue, Nov 6, 2012 at 7:52 AM, Jason Merrill <jason@redhat.com> wrote:
> On 11/05/2012 09:38 PM, Sriraman Tallam wrote:>>>> + /* For multi-versioned functions, more than one match is just fine.>>>> + Call decls_match to make sure they are different because they are>> + versioned. */>> + if (DECL_FUNCTION_VERSIONED (fn))>> + {>> + for (match = TREE_CHAIN (matches); match; match = TREE_CHAIN>> (match))>> + if (!DECL_FUNCTION_VERSIONED (TREE_PURPOSE (match))>> + || decls_match (fn, TREE_PURPOSE (match)))>> + break;>> + }>>> I still don't understand what this code is supposed to be doing. Please> remove it and instead modify the other loop to allow mismatches that are> versions of the same function.
Ok, will do. I was trying to do for versioned functions what the other
loop was doing thought I could not come up with a test case to
exercise this code.
I will make all the other changes and get back asap.
Thanks,
-Sri.
>>> + /* If the olddecl is a version, so is the newdecl. */>> + if (TREE_CODE (newdecl) == FUNCTION_DECL>> + && DECL_FUNCTION_VERSIONED (olddecl))>> + {>> + DECL_FUNCTION_VERSIONED (newdecl) = 1;>> + /* newdecl will be purged and is no longer a version. */>> + delete_function_version (newdecl);>> + }>>> Please make the comment clearer that the reason we're setting the flag on> the newdecl is so that it'll be copied back into the olddecl; otherwise it> seems odd to say it's a version and then it isn't a version.>>> + /* If a pointer to a function that is multi-versioned is requested, the>> + pointer to the dispatcher function is returned instead. This works>> + well because indirectly calling the function will dispatch the right>> + function version at run-time. */>>>> + if (DECL_FUNCTION_VERSIONED (fn))>> + {>> + tree dispatcher_decl = NULL;>> + gcc_assert (targetm.get_function_versions_dispatcher);>> + dispatcher_decl = targetm.get_function_versions_dispatcher (fn);>> + if (!dispatcher_decl)>> + {>> + error_at (input_location, "Pointer to a multiversioned function">> + " without a default is not allowed");>> + return error_mark_node;>> + }>> + retrofit_lang_decl (dispatcher_decl);>> + fn = dispatcher_decl;>>> This code should use the get_function_version_dispatcher function in> cp/call.c.>> Jason>