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?
+ {
+ 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?
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.
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
