rsmith added a comment.
I expect there are more places that need to be changed, but this is looking
surprisingly clean.
You will need to teach the modules merging code that it needs to check this
attribute in addition to checking that types match when considering merging
declaration chains for functions. (See `isSameEntity` in
Serialization/ASTReaderDecl.cpp).
I don't see any tests for non-call uses of multiversioned functions (eg, taking
their address, binding function references to them). Does that work? (I'd
expect you to need "ignore non-default versions" logic in more places for that.)
================
Comment at: include/clang/AST/Decl.h:1754-1756
+ /// Indicates that this function is a multiversioned function using attribute
+ /// 'target'.
+ unsigned IsMultiVersion : 1;
----------------
This flag needs serialization support for PCH / modules, and you'll need to do
something if we find declarations with inconsistent values for the flag across
modules (detect and diagnose, probably), and likewise in chained PCH (where the
right thing to do is presumably to update all prior declarations to have the
same value for the flag).
================
Comment at: include/clang/Basic/Attr.td:1809
bool DuplicateArchitecture = false;
+ bool operator ==(const ParsedTargetAttr &Other) {
+ return DuplicateArchitecture == Other.DuplicateArchitecture &&
----------------
Our normal convention is to not put a space before `==` here.
================
Comment at: include/clang/Basic/DiagnosticSemaKinds.td:9317-9318
+def err_target_required_in_redecl :
+ Error<"function declaration is missing 'target' attribute in a "
+ "multiversioned function">;
+def note_multiversioning_caused_here :
----------------
There's some weird indentation hereabouts. Our normal convention is to put the
`Error<` on the `def` line, and start the diagnostic text as a 2-space-indented
string literal on the next line.
================
Comment at: include/clang/Basic/TargetInfo.h:927
+ virtual unsigned multiVersionSortPriority(StringRef Name) const {
+ return false;
+ }
----------------
`return 0;`
================
Comment at: lib/CodeGen/CodeGenModule.cpp:2141
+
+ for (auto *CurDecl : FD->getDeclContext()->lookup(FD->getIdentifier())) {
+ if (const auto *CurFD = dyn_cast<FunctionDecl>(CurDecl))
----------------
You need to call `getRedeclContext()` on the value returned by
`getDeclContext()` to skip "transparent" `DeclContext`s such as
`LinkageSpecDecl` and C++ Modules TS `export` declarations.
================
Comment at: lib/CodeGen/CodeGenModule.cpp:2141
+
+ for (auto *CurDecl : FD->getDeclContext()->lookup(FD->getIdentifier())) {
+ if (const auto *CurFD = dyn_cast<FunctionDecl>(CurDecl))
----------------
rsmith wrote:
> You need to call `getRedeclContext()` on the value returned by
> `getDeclContext()` to skip "transparent" `DeclContext`s such as
> `LinkageSpecDecl` and C++ Modules TS `export` declarations.
`getIdentifier` here is wrong; use `getDeclName()` instead, to correctly handle
non-identifier names.
================
Comment at: lib/CodeGen/CodeGenModule.cpp:2144
+ if (getContext().hasSameType(CurFD->getType(), FD->getType())) {
+ StringRef MangledName = getMangledName(CurFD);
+ llvm::Constant *Func = GetGlobalValue(MangledName);
----------------
You should skip functions you've already seen somewhere around here; we don't
need to handle the same function multiple times.
================
Comment at: lib/Sema/SemaDecl.cpp:9326
+
+static bool CheckMultiVersionAdditionalRules(Sema &S, const FunctionDecl
*OldFD,
+ const FunctionDecl *NewFD,
----------------
rsmith wrote:
> Would it be possible to factor out the checks in `MergeFunctionDecl` that
> you're using here and reuse them directly?
Maybe also check that the language linkage matches (`extern "C"` and `extern
"C++"` could imply different calling conventions, even though they don't on any
of our current targets).
================
Comment at: lib/Sema/SemaDecl.cpp:9326-9328
+static bool CheckMultiVersionAdditionalRules(Sema &S, const FunctionDecl
*OldFD,
+ const FunctionDecl *NewFD,
+ bool CausesMV) {
----------------
Would it be possible to factor out the checks in `MergeFunctionDecl` that
you're using here and reuse them directly?
================
Comment at: lib/Sema/SemaDecl.cpp:9383-9384
+
+ QualType OldReturnType = OldType->getReturnType();
+ QualType NewReturnType = NewType->getReturnType();
+ if (OldReturnType != NewReturnType) {
----------------
This won't do the right thing for C++14 deduced return types. I'm not
completely sure what the right thing is, though -- clearly we need to deduce
the same return type in all versions of the function, but it's less clear
whether we should also require the return-type-as-written to match across
versions (as is required across redeclarations). Example:
```
VERSION_FOO auto f() { return 0; } // declared return type is `auto`, actual
return type is `int`
VERSION_BAR auto f(); // will fail the current check, because the return type
`auto` doesn't match the prior return type `int`
VERSION_BAR auto f() { return 0.0; } // should reject this because we deduced a
different return type than on the VERSION_FOO version?
```
Perhaps the simplest thing would be to simply disallow deduced return types for
multiversioned functions entirely for now?
================
Comment at: lib/Sema/SemaDecl.cpp:9431-9432
+ LookupResult &Previous) {
+ if (NewFD->isMain())
+ return false;
+ const auto *NewTA = NewFD->getAttr<TargetAttr>();
----------------
Should we reject (eg) `__attribute__((target("default")))` on `main`?
================
Comment at: lib/Sema/SemaDecl.cpp:9435
+
+ // If there is no matching previous decl, than only 'default' can
+ // cause MultiVersioning.
----------------
Typo "than"
================
Comment at: lib/Sema/SemaDecl.cpp:9459-9460
+
+ if (OldFD->isMain())
+ return false;
+
----------------
Can this happen if `NewFD` is not `main`?
================
Comment at: lib/Sema/SemaDecl.cpp:9539-9540
+ // previous member of the MultiVersion set.
+ for (NamedDecl *ND : Previous) {
+ auto *CurFD = ND->getAsFunction();
+ if (!CurFD)
----------------
We should be a little careful here: we don't want to allow multiversioning
between declarations in different semantic `DeclContext`s:
```
VERSION_DEFAULT void f();
namespace N {
using ::f;
VERSION_FOO void f();
}
```
... because when we generate the dispatcher, we won't find the right set of
versions. The easiest way to deal with this might be to check near the start of
this function whether the semantic `DeclContext` of `CurFD` and `OldFD` are
equal, and if not, just bail out and leave the redeclaration merging code to
diagnose the conflict. (We already prevent a `DeclContext` from containing two
non-overloadable functions where one of them is 'using'd from another
`DeclContext` and the other is declared locally.)
https://reviews.llvm.org/D40819
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
