[PATCH] D38596: Implement attribute target multiversioning

[Erich Keane via Phabricator via cfe-commits]

erichkeane added a comment.

In https://reviews.llvm.org/D38596#913322, @rsmith wrote:

> The big pain point here is that we don't know whether a function is multiversioned until we see the *second* version of it (or a declaration with no target attribute or a declaration with a "default" target), and we generally want our AST to be immutable-once-created. I don't see a good way to handle this except by mutating the AST when we see the second version (which leads to complexity, particularly around PCH -- you need to inform AST mutation listeners that this has happened and write a record out into the AST file to indicate you've updated an imported entity) -- or by treating any function with a target attribute as if it were multiversioned, and not treating a declaration with a body as being the definition until / unless we see the first use of it (which, again, leads to an AST mutation of some kind at that point).
>
> Keeping the redeclaration chains separate would avoid this problem, but we'd still need to do *something* to track the set of versions of the function so we can emit them as part of the ifunc.


That is definitely a pain point.  The solution I've had bouncing around in my head is the following, but it has some limitations that GCC doesn't have:
1- Convert to multiversion not permitted unless 1 of the 2 is 'default' (since it is required).
2- Convert to multiversion not permitted if the decl has been used.
3- Prevent the 'lookup' from finding the non-default version, this permits things like constexpr to work properly.
4- Add a list of the "Multiversion Decls" to the 'default' FunctionDecl.

My hope being that #2 makes it so that no one would really NOTICE the AST rewriting, since it hasn't been used.  
#1 enables #3, so that the only 'user' of the multiversion'ed decls ends up being the CodeGen emitting the ifunc.
#4 of course is to give CodeGen the ability to emit all of the definitions.

Is there a better way to 'downgrade' the non-default FunctionDecl's to 'not lookupable'?  Or is this something I would need to add?

Finally, 1 further idea is that "default" automatically creates it as a 'multiversion case'.  Since a sole 'default' would be invalid in GCC, I think that makes us OK, right?

Thoughts on the above proposal?  I have basically abandoned this review, since the changes are pretty intense.  I've also submitted another patch (that Craig Topper is helping me with) to cleanup the CPU/Feature definitions to make that list more sane.

BTW: Just saw your 2nd response: I think the rejection of cases where multiversioning is caused after first-use is the only real way to make this sane.

Thanks again for your input!


https://reviews.llvm.org/D38596