On 10/08/2015 02:01 PM, Evgeny Stupachenko wrote:
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.
Seems like a comment would be wise.
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.
Thanks. Not sure why, but I'd appreciate that change.
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.
Right, but there's nothing inherently that says that a option couldn't
have other operators such as '+' in the option string. So I'm concerned
that if this code were used on a target that had such an option that
we'd end up generating invalid assembly.
I think all you have to do is map a fuller set of invalid characters to
a valid character.
Jeff
