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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
