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

[Sriraman Tallam <tmsriram at google dot com>]

Hi Jason,

   I have attached the latest patch with more cleanups. Please let me
know what you think.

   Honza, can you please review the cgraph part?

Thanks,
-Sri.

On Wed, Oct 10, 2012 at 4:45 PM, Sriraman Tallam <tmsriram@google.com> wrote:
> Hi Jason,
>
>    I have addressed all your comments and attached the new patch.
>
> On Fri, Oct 5, 2012 at 11:32 AM, Jason Merrill <jason@redhat.com> wrote:
>> On 08/24/2012 08:34 PM, Sriraman Tallam wrote:
>>>
>>> +         /* For function versions, their parms and types match
>>> +            but they are not duplicates.  Record function versions
>>> +            as and when they are found.  */
>>> +         if (TREE_CODE (fn) == FUNCTION_DECL
>>> +             && TREE_CODE (method) == FUNCTION_DECL
>>> +             && (DECL_FUNCTION_SPECIFIC_TARGET (fn)
>>> +                 || DECL_FUNCTION_SPECIFIC_TARGET (method))
>>> +             && targetm.target_option.function_versions (fn, method))
>>> +           {
>>> +             targetm.set_version_assembler_name (fn);
>>> +             targetm.set_version_assembler_name (method);
>>> +             continue;
>>> +           }
>>
>>
>> This seems like an odd place to be setting assembler names; better to just
>> have the existing mangle_decl_assembler_name hook add the appropriate suffix
>> when it's called normally.
>
> I moved this to mangle_decl_assembler_name. Still,  functions may go
> from not being a version to then becoming versions after a new
> definition is detected. In such cases, I explicitly call mangle_decl
> to modify the assembler name.
>
>>
>>
>>> +        Also, mark this function as needed if it is marked inline but
>>> +        is a multi-versioned function.  */
>>
>>
>> Why?  If it's used, it should be marked needed though the normal process.
>
> How do I do this? If a versioned function is marked inline, I need to
> keep it but it has no explicit callers. How do I mark that it is
> needed?
>
>>
>>> +           error_at (location_of (DECL_NAME (OVL_CURRENT (fn))),
>>> +                     "Call to multiversioned function %<%D(%A)%> with"
>>> +                     " no default version", DECL_NAME (OVL_CURRENT (fn)),
>>> +                     build_tree_list_vec (*args));
>>
>>
>> location_of just returns input_location if you ask it for the location of an
>> identifier, so you might as well use error with no explicit location.  And
>> why not print candidates->fn instead of pasting the name/args?  Also,
>> lowercase "call".
>
> I removed this since the check already happens elsewhere.
>
>>
>>> +    {
>>> +      tree dispatcher_decl = NULL;
>>> +      struct cgraph_node *node = cgraph_get_node (fn);
>>> +      if (node != NULL)
>>> +        dispatcher_decl = cgraph_get_node (fn)->version_dispatcher_decl;
>>> +      if (dispatcher_decl == NULL)
>>> +       {
>>> +         error_at (input_location, "Call to multiversioned function"
>>> +                   " without a default is not allowed");
>>> +         return NULL;
>>> +       }
>>> +      retrofit_lang_decl (dispatcher_decl);
>>> +      gcc_assert (dispatcher_decl != NULL);
>>> +      fn = dispatcher_decl;
>>> +    }
>>
>>
>> Let's move this logic into a separate function that returns the dispatcher
>> function.
>
> Done.
>
>>
>>> +      /* Both functions must be marked versioned.  */
>>> +      gcc_assert (DECL_FUNCTION_VERSIONED (cand1->fn)
>>> +                 && DECL_FUNCTION_VERSIONED (cand2->fn));
>>
>>
>> Why can't you compare a versioned function and a non-versioned one?
>
> Right, there was a big bug in my code. I have changed this now. This
> should address your question.
>
>>
>> The code in joust should go further down in the function, before the
>> handling of two declarations of the same function.
>
> Done.
>
>>
>>> +  /* For multiversioned functions, aggregate all the versions here for
>>> +     generating the dispatcher body later if necessary.  */
>>> +
>>> +  if (TREE_CODE (candidates->fn) == FUNCTION_DECL
>>> +      && DECL_FUNCTION_VERSIONED (candidates->fn))
>>> +    {
>>>
>>> +      VEC (tree, heap) *fn_ver_vec = NULL;
>>> +      struct z_candidate *ver = candidates;
>>>
>>> +      fn_ver_vec = VEC_alloc (tree, heap, 2);
>>> +      for (;ver; ver = ver->next)
>>> +        VEC_safe_push (tree, heap, fn_ver_vec, ver->fn);
>>> +      gcc_assert (targetm.get_function_versions_dispatcher);
>>> +      targetm.get_function_versions_dispatcher (fn_ver_vec);
>>> +      VEC_free (tree, heap, fn_ver_vec);
>>> +    }
>>
>>
>> This seems to assume that all the functions in the list of candidates are
>> versioned, but there might be unrelated functions from different namespaces.
>> Also, doing this every time someone calls a versioned function seems like
>> the wrong place; I would think it would be better to build up a list of
>> versions as you seed declarations, and then use that list to define the
>> dispatcher at EOF if it's needed.
>
>
> This was the bug I was referring to earlier. I have moved this to a
> separate function. I thought it is better to do this on demand. I have
> changed the code so that the aggregation and dispatcher generation
> happens exactly once.
>
>
>>
>>> +      if (TREE_CODE (decl) == FUNCTION_DECL
>>> +          && DECL_FUNCTION_VERSIONED (decl)
>>> +         && DECL_ASSEMBLER_NAME_SET_P (decl))
>>> +       write_source_name (DECL_ASSEMBLER_NAME (decl));
>>> +      else
>>> +       write_source_name (DECL_NAME (decl));
>>
>>
>> Again, I think it's better to handle the suffix via
>> mangle_decl_assembler_name.
>
> Removed.
>
>
> Thanks for the comments. Please let me know what you think about the new patch.
>
> -Sri.
>
>>
>> Jason
>>

Attachment: mv_fe_patch_10122012.txt
Description: Text document