rsmith added a comment.
One other test I'd like to see is what happens if you declare a version before
the first use, and define it afterwards, particularly if the version is an
inline function:
inline __attribute__((target("default"))) void f();
inline __attribute__((target("foo"))) void f();
void g() { f(); }
__attribute__((target("default"))) void f() {}
__attribute__((target("foo"))) void f() {}
================
Comment at: include/clang/AST/ASTContext.h:2643-2648
+ for (auto *CurDecl :
+ FD->getDeclContext()->getRedeclContext()->lookup(FD->getDeclName())) {
+ FunctionDecl *CurFD = CurDecl->getAsFunction()->getCanonicalDecl();
+ if (CurFD && hasSameType(CurFD->getType(), FD->getType()))
+ P(CurFD);
+ }
----------------
This should deal with the case where `lookup` finds multiple declarations of
the same version (which can happen in particular when serializing to AST files;
we keep around a handle to the version from each AST file). Also, consider
moving this to the .cpp file (use `llvm::function_ref<void(const
FunctionDecl*)>` to pass in the callback).
================
Comment at: include/clang/Basic/Attr.td:1809
bool DuplicateArchitecture = false;
+ bool operator==(const ParsedTargetAttr &Other) {
+ return DuplicateArchitecture == Other.DuplicateArchitecture &&
----------------
rsmith wrote:
> aaron.ballman wrote:
> > This function should be marked `const`.
> Nit: space before `{`.
Space before `{` seems to have been removed again. Clang-format really doesn't
like tablegen files :)
================
Comment at: include/clang/Basic/DiagnosticSemaKinds.td:9354-9356
+ "multiversioned function declaration has a different %select{calling
convention"
+ "|return type|constexpr specification|inline specification|storage class|"
+ "linkage|destructors}0">;
----------------
"multiversioned function declaration has a different destructors" doesn't sound
right to me :)
================
Comment at: lib/Sema/SemaDecl.cpp:9228-9229
+ S.Diag(NewFD->getLocation(), diag::err_multiversion_no_other_attrs);
+ if (CausesMV)
+ S.Diag(NewFD->getLocation(), diag::note_multiversioning_caused_here);
+ return true;
----------------
It seems strange to have a "caused here" note pointing at the same location
that you just produced an error on. Is this note really adding value? (Likewise
on all later diagnostics that add this note at the same place as the error.)
================
Comment at: lib/Sema/SemaOverload.cpp:10220
+ case ovl_non_default_multiversion_function:
+ // Do Nothing, these should simly be ignored.
+ break;
----------------
Couple of typos here.
================
Comment at: lib/Serialization/ASTReaderDecl.cpp:2822
+ const auto *TAY = FuncY->getAttr<TargetAttr>();
+ assert(TAX && TAY && "Multiversion Function without target attribute");
+
----------------
rsmith wrote:
> This assertion will fire if X is multiversion but Y is not. It's probably not
> too crucial what happens in that case, but we shouldn't assert. An error
> would be nice, but we can't produce one from here, so how about we just say
> that if one is multiversion and the other is not, that they're considered to
> not be the same entity.
This is still not quite right for the case of a multiversion / non-multiversion
mismatch: you don't return 'false' when Y is multiversion and X is not.
================
Comment at: test/CodeGenCXX/attr-target-mv-constexpr.cpp:1-16
+// RUN: %clang_cc1 -std=c++11 -triple x86_64-linux-gnu -emit-llvm %s -o - |
FileCheck %s
+
+constexpr int __attribute__((target("sse4.2"))) foo(void) { return 0; }
+constexpr int __attribute__((target("arch=sandybridge"))) foo(void);
+constexpr int __attribute__((target("arch=ivybridge"))) foo(void) {return 1;}
+constexpr int __attribute__((target("default"))) foo(void) { return 2; }
+
----------------
Can you move this to test/SemaCXX and change it to only test the constant
evaluation side of things (eg, `static_assert(foo() == 2)`)?
https://reviews.llvm.org/D40819
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
