[PATCH] D38596: Implement attribute target multiversioning

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added a comment.

In https://reviews.llvm.org/D38596#910035, @erichkeane wrote:

> > You should add tests for:
> > 
> > - the PCH / Modules case (particularly regarding what happens when merging multiple copies of the same set of functions, for instance when they're defined inline); I would expect this doesn't currently work with this patch, because we only ever load one function body per redeclaration chain
>
> I'm not sure I know what one of those looks like... is there a good test that I can use to learn the modules implementation?


There are a bunch in `test/PCH` and `test/Modules` that you could crib from.

>> - multiversioned functions declared inline
> 
> I don't see anything changing for this situation, but I imagine you have an issue in mind that I'm missing?

You need to make sure you use the correct linkage for the versions and for the dispatcher. Your tests seem to demonstrate you're getting this wrong at the moment. Also, this is the case where the mangling of the target-specific definitions would become part of the ABI.

>> - constant expression evaluation of calls to such functions (which should -- presumably -- either always use the default version or be rejected as non-constant)
> 
> I'd say they should always do the default version, but this isn't something I did any work on.  I'll look at that.
> 
>> I would also expect that asking a multiversioned `FunctionDecl` for its definition would only ever give you the `target("default")` definition or the one-and-only definition, rather than an arbitrary declaration that happens to have a body.
> 
> Right, this is a big catch that I didn't think about...

What I'm essentially thinking here is that a declaration with a body for a multiversioned function, that isn't for the "default" target, would not be treated as being "the definition" at all.

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.


https://reviews.llvm.org/D38596