> 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