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
>