On Thu, Oct 8, 2015 at 10:00 PM, Jeff Law <l...@redhat.com> wrote:
> On 09/24/2015 04:28 PM, Evgeny Stupachenko wrote:
>>
>> I've fixed ICE and review issues.
>> x86 make check and bootstrap passed.
>>
>> Thanks,
>> Evgeny
>>
>> ChangeLog
>>
>> 2015-09-25 Evgeny Stupachenko<evstu...@gmail.com>
>>
>> gcc/
>> * Makefile.in (OBJS): Add multiple_target.o.
>> * multiple_target.c (make_attribute): New.
>> (create_dispatcher_calls): Ditto.
>> (expand_target_clones): Ditto.
>> (ipa_target_clone): Ditto.
>> * passes.def (pass_target_clone): New ipa pass.
>> * tree-pass.h (make_pass_target_clone): Ditto.
>>
>> gcc/c-family
>> * c-common.c (handle_target_clones_attribute): New.
>> * (c_common_attribute_table): Add handle_target_clones_attribute.
>> * (handle_always_inline_attribute): Add check on target_clones
>> attribute.
>> * (handle_target_attribute): Ditto.
>>
>> gcc/testsuite
>> * gcc.dg/mvc1.c: New test for multiple targets cloning.
>> * gcc.dg/mvc2.c: Ditto.
>> * gcc.dg/mvc3.c: Ditto.
>> * gcc.dg/mvc4.c: Ditto.
>> * gcc.dg/mvc5.c: Ditto.
>> * gcc.dg/mvc6.c: Ditto.
>> * gcc.dg/mvc7.c: Ditto.
>> * g++.dg/ext/mvc1.C: Ditto.
>> * g++.dg/ext/mvc2.C: Ditto.
>> * g++.dg/ext/mvc3.C: Ditto.
>>
>> gcc/doc
>> * doc/extend.texi (target_clones): New attribute description.
>>
>>
>
>>
>> target_clones.patch
>
> Sorry this has taken so long to come back to... As I mentioned a couple
> months ago, I'd hoped Jan would chime in on the IPA/symtab requirements.
> But that didn't happen.
>
>
> SO I went back and reviewed the discussion between Jan, Ilya & myself WRT
> some of the rules around aliases, clones, etc. I think the key question for
> this patch is whether or not the clones have the same assembler name or not.
> From looking at expand_target_clones, I'm confident the answer is the clones
> have different assembler names. In fact, the assembler names are munged
> with the options used for that specific clone of the original function.
>
>
>>
>> +/* Makes a function attribute of the form NAME(ARG_NAME) and chains
>> + it to CHAIN. */
>> +
>> +static tree
>> +make_attribute (const char *name, const char *arg_name, tree chain)
>> +{
>> + tree attr_name;
>> + tree attr_arg_name;
>> + tree attr_args;
>> + tree attr;
>> +
>> + attr_name = get_identifier (name);
>> + attr_arg_name = build_string (strlen (arg_name), arg_name);
>> + attr_args = tree_cons (NULL_TREE, attr_arg_name, NULL_TREE);
>> + attr = tree_cons (attr_name, attr_args, chain);
>> + return attr;
>> +}
>
> This seems generic enough that I'd prefer it in attribs.c. I was rather
> surprised when I looked and didn't find an existing routine to do this.
>
>
>> +
>> +/* If the call in NODE has multiple target attribute with multiple
>> fields,
>> + replace it with dispatcher call and create dispatcher (once). */
>> +
>> +static void
>> +create_dispatcher_calls (struct cgraph_node *node)
>> +{
>> + cgraph_edge *e;
>> + cgraph_edge *e_next;
>> + for (e = node->callers; e ;e = (e == NULL) ? e_next : e->next_caller)
>
> That's a rather strange way to write the loop increment. If I follow the
> loop logic correctly, it seems that we always end up using e->next_caller,
> it's just obscured.
>
> For the test if we're calling a versioned function, we just "continue". e
> will be non-null and thus we use e->next_caller to set e for the next
> iteration.
>
> If the test for calling a versioned function falls, we set e_next to
> e->next_caller, then later set e to NULL. That results in using e_next to
> set e for the next iteration. But e_next was initialized to e->next_caller.
>
> So why not just write the loop increment as e = e->next-caller?
Because of this:
e->redirect_callee (inode);
It modifies next_caller field.
The other way is to remember all what we want to redirect and create
one more loop through the modifications to apply them.
>
>
>
>> + {
>> + tree resolver_decl;
>> + tree idecl;
>> + tree decl;
>> + gimple *call = e->call_stmt;
>> + struct cgraph_node *inode;
>> +
>> + /* Checking if call of function is call of versioned function.
>> + Versioned function are not inlined, so there is no need to
>> + check for inline. */
>
> This comment doesn't parse well. Perhaps:
>
> /* Check if this is a call to a versioned function. Verisoned
> fucntions are not inlined, so there is no need to check for that. */
>
>
>> +
>> +/* If the function in NODE has multiple target attribute with multiple
>> fields,
>> + create the appropriate clone for each field. */
>> +
>> +static bool
>> +expand_target_clones (struct cgraph_node *node)
>
> So this is probably getting a little large. Can we look to refactor it a
> little? It's not a huge deal and there's certainly code in GCC that is far
> worse, but it just feels like there's enough going on in this code that
> there ought to be 2-3 helpers for the larger chunks of work going on inside
> this code.
>
> The first is the attribute parsing. The second is creation of the nodes,
> and the final would be munging the name and attributes on the clones.
>
> I'm slightly concerned with using the pretty printer to build up the new
> name. Is there precedent for this anywhere else in GCC?
I don't remember where it exactly came from. However it's not a big
deal to simplify this to std functions.
>
> When creating the munged name, don't you also have to make sure that other
> symbols that aren't supported for names don't sneak through? I see that you
> replace = and -, but you'd need to replace any symbol that could possibly be
> used in an option, but which isn't allowed in a function name at the
> assembler level. I'd be worried about anything that might possibly be seen
> as an operator by the assembler, '.', and possibly others.
This restriction comes from "targetm.target_option.valid_attribute_p"
and it is the same for current implementation of function
multiversioning.
It exits with error: "attribute(target("...")) is unknown".
It looks reasonable to put the check before symtab changes.
>
> Also, please double check indentation of that function. In particular the
> code after the if-then-else (node->definition) looks like it's indented too
> far.
>
> I think this is pretty close, but does need another iteration. Now that
> we've moved past the interactions with IPA question and have fairly
> carefully looked at the rest of the patch, I don't think subsequent reviews
> will take much time at all.
>
> jeff
Thanks for the review,
Evgeny
