Re: [PATCH] New attribute to create target clones

[Jan Hubicka <hubicka@ucw.cz>]

> 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.

Sorry for that.  I had bit too many real life things this summer
and I am still trying to catch up.
> 
> 
> 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

Ilya's code seems different from what this patch does. Ilya simply needs
multiple declarations for one physical assembler name (this is not an alias).
This is not currently supported by symtab (support for that was removed long
time ago as part of the one decl project) and I have some perliminary patches
to push out, but since they add basic sanity checking that the different
declarations of the same thing looks compatible I get too many positives I need
to walk through.  Those seems real bugs in glibc (which uses duplicated decls
for checking) and the pointer bounds code.

> expand_target_clones, I'm confident the answer is the clones have
> different assembler names.  In fact, the assembler names are munged

Yes, here you have different names for different variants of the function
body. Basically this pass takes ctarget attribute and creates bunch of verisons
of the functions and assigns them the proper target attributes, right?

One thing I am confused about is why this does not happen early?
What happens to inlines of functions with specific taret requirements? I.e.
if I compile with AVX enabled have function body with AVX code but then, at
late compilation, force a clone with -mno-avx?

I would expect cloning to happen early, perhaps even before early optimizations...
Switching random target flags mid optimization queue seems dangerous.

As for the patch itself:
+      if (node->definition)
+	{
+	  if (!node->has_gimple_body_p ())
+	    return false;
+	  node->get_body ();
+
+	  /* Replacing initial function with clone only for 1st ctarget.  */
+	  new_node = node->create_version_clone_with_body (vNULL, NULL,
+							   NULL, false,
+							   NULL, NULL,
+							   "target_clone");
+	  new_node->externally_visible = node->externally_visible;
+	  new_node->address_taken = node->address_taken;
+	  new_node->thunk = node->thunk;
+	  new_node->alias = node->alias;
+	  new_node->weakref = node->weakref;
+	  new_node->cpp_implicit_alias = node->cpp_implicit_alias;
+	  new_node->local.local = node->local.local;
+	  TREE_PUBLIC (new_node->decl) = TREE_PUBLIC (node->decl);
+	}
Since you test it has gimple body, then you don't need to worry about
alias/thunk/weakrefs/implicit_alias properties. Those will be never set.
How does the dispatcher look like?  Can the function be considered local
in a sense that one can change calling conventions of one clone but not another?

On the other hand, node->create_version_clone_with_body creates a function
that is local, if we want to create globally visible clones, I think we should
add a parameter there and avoid the localization followed by unlocalization.
(I know we already have too many parameters here, perhaps we could add a flags
parameter that will also handle the existing skip_return flag.)
For exmaple, I think you want to have all functions with same WEAK attributes
or in the same COMDAT group.

Also when you are copying a function, you probably want to copy the associated
thunks and version them, too?

+      id = get_identifier (new_asm_name);
+      symtab->change_decl_assembler_name (new_node->decl, id);

here I think you can just pass new_asm_name as clone_name. I.e. replace
the current use of "target_clone" string.

+      targetm.target_option.valid_attribute_p (new_node->decl, NULL,
+					       TREE_VALUE (attributes), 0);

looks like return value should not be ignored. If attribute is not valid,
we need to error and do something sane.

Honza