rsmith added a comment.
Lots of comments, but no high-level design concerns. I think this is very close
to being ready to go.
================
Comment at: include/clang/AST/Decl.h:2162-2168
+ /// Sets the multiversion state for this declaration and all of its
+ /// redeclarations.
+ void setIsMultiVersion(bool V = true) {
+ IsMultiVersion = V;
+ for (FunctionDecl *FD : redecls())
+ FD->IsMultiVersion = V;
+ }
----------------
Instead of looping over redeclarations here, how about only storing the flag on
the canonical declaration (and only looking there in `isMultiVersion`)?
================
Comment at: include/clang/Basic/Attr.td:1809
bool DuplicateArchitecture = false;
+ bool operator==(const ParsedTargetAttr &Other) {
+ return DuplicateArchitecture == Other.DuplicateArchitecture &&
----------------
aaron.ballman wrote:
> This function should be marked `const`.
Nit: space before `{`.
================
Comment at: include/clang/Basic/DiagnosticSemaKinds.td:9319-9320
+def err_target_required_in_redecl : Error<
+ "function declaration is missing 'target' attribute in a multiversioned "
+ "function">;
+def note_multiversioning_caused_here : Note<
----------------
Is it "multiversion function" or "multiversioned function"? You're using both
in these diagnostics; please pick one and use it consistently. I prefer the
"-ed" form, but I'm happy with whichever you'd prefer.
================
Comment at: include/clang/Basic/DiagnosticSemaKinds.td:9322
+def note_multiversioning_caused_here : Note<
+ "function multiversion caused by this declaration">;
+def err_multiversion_after_used : Error<
----------------
"multiversioning" maybe? (Again, this is used inconsistently; there's a
"function multiversioning" in err_multiversion_not_supported).
================
Comment at: include/clang/Basic/DiagnosticSemaKinds.td:9329-9331
+def note_ovl_candidate_nondefault_multiversion : Note<
+ "candidate ignored: non-default multiversion function cannot be called "
+ "directly">;
----------------
Maybe we should just suppress the "candidate" note entirely for these cases?
================
Comment at: lib/CodeGen/CodeGenModule.cpp:2144
+ if (getContext().hasSameType(CurFD->getType(), FD->getType())) {
+ StringRef MangledName = getMangledName(CurFD);
+ llvm::Constant *Func = GetGlobalValue(MangledName);
----------------
erichkeane wrote:
> rsmith wrote:
> > You should skip functions you've already seen somewhere around here; we
> > don't need to handle the same function multiple times.
> Will the lookup call give me duplicates? I need to check each at least once
> through this so that I can put them into "Options" so that I can emit them
> during the resolver.
>
> Otherwise, I'm not terribly sure what you mean.
Yes, the lookup can in some cases find multiple declarations from the same
redeclaration chain. (This happens particularly when the declarations are
loaded from AST files.)
================
Comment at: lib/CodeGen/CodeGenModule.cpp:840-841
+
+ const auto *ND = cast<NamedDecl>(GD.getDecl());
+ UpdateMultiVersionNames(GD, ND);
+
----------------
I'm not especially enamoured with `getMangledName` mutating the IR. Can we
perform this rename as part of emitting the multiversion dispatcher or ifunc,
rather than here? (Or do we really not have anywhere else that this can live?)
================
Comment at: lib/CodeGen/CodeGenModule.cpp:2056-2057
const auto *F = cast<FunctionDecl>(GD.getDecl());
+ if (F->isMultiVersion())
+ return true;
if (CodeGenOpts.OptimizationLevel == 0 && !F->hasAttr<AlwaysInlineAttr>())
----------------
Please add a comment explaining this check. (I'm guessing the issue is that we
can't form a cross-translation-unit reference to the right function from an
IFUNC, so we can't rely on an available_externally definition actually being
usable? But it's not clear to me that this is the right resolution to that,
especially in light of the dllexport case below where it would not be correct
to emit the definition.)
================
Comment at: lib/CodeGen/CodeGenModule.cpp:2140-2143
+ for (auto *CurDecl : FD->getDeclContext()->getRedeclContext()->lookup(
+ FD->getDeclName())) {
+ if (const auto *CurFD = dyn_cast<FunctionDecl>(CurDecl))
+ if (getContext().hasSameType(CurFD->getType(), FD->getType())) {
----------------
Consider moving the lookup, loop and type check here into an ASTContext or
FunctionDecl utility to find or visit all the versions of a multiversioned
function.
================
Comment at: lib/CodeGen/CodeGenModule.cpp:2142
+ FD->getDeclName())) {
+ if (const auto *CurFD = dyn_cast<FunctionDecl>(CurDecl))
+ if (getContext().hasSameType(CurFD->getType(), FD->getType())) {
----------------
`if` statements with nontrivial bodies should have their own braces.
================
Comment at: lib/Sema/SemaDecl.cpp:9526
+ S.Diag(NewFD->getLocation(), diag::err_multiversion_not_supported);
+ S.Diag(OldFD->getLocation(), diag::note_previous_decl) << "previously";
+ return true;
----------------
Do not hardcode English text as a diagnostic parameter. And don't reuse a
diagnostic to mean something else :) The %0 in this diagnostic is for the name
of the declaration.
`diag::note_previous_declaration` seems to capture what you're actually looking
for here. (It's a note for "previous declaration of the same entity", whereas
`note_previous_decl` is a note for "here's some previous declaration of
something else that's relevant").
================
Comment at: lib/Sema/SemaDecl.cpp:9531-9532
+ if (!NewFD->getType()->getAs<FunctionProtoType>()) {
+ S.Diag(OldFD->getLocation(), diag::err_multiversion_noproto);
+ S.Diag(NewFD->getLocation(), diag::note_multiversioning_caused_here);
+ NewFD->setInvalidDecl();
----------------
This doesn't seem right: the OldFD isn't (necessarily) a declaration without a
prototype.
================
Comment at: lib/Sema/SemaDecl.cpp:9553
+ S.Diag(NewFD->getLocation(), diag::err_multiversion_duplicate);
+ S.Diag(OldFD->getLocation(), diag::note_previous_decl) << "previously";
+ NewFD->setInvalidDecl();
----------------
As above.
================
Comment at: lib/Sema/SemaDecl.cpp:9607
+ S.Diag(NewFD->getLocation(), diag::err_multiversion_duplicate);
+ S.Diag(CurFD->getLocation(), diag::note_previous_decl) << "previously";
+ NewFD->setInvalidDecl();
----------------
As above.
================
Comment at: lib/Sema/SemaDecl.cpp:9720
+ if (CheckMultiVersionFunction(*this, NewFD, Redeclaration, OldDecl,
+ MergeTypeWithPrevious, Previous))
+ return Redeclaration;
----------------
The parameter in `CheckMultiVersionFunction` corresponding to
`MergeTypeWithPrevious` here is called `MayNeedOverloadableChecks`, which is
also the name of a local variable in this function. Did you pass the wrong
bool? Or is the name of the parameter to `CheckMultiVersionFunction` wrong?
================
Comment at: lib/Sema/SemaOverload.cpp:5944
+ if (Function->isMultiVersion() &&
+ Function->getAttr<TargetAttr>()->getFeaturesStr() != "default") {
+ Candidate.Viable = false;
----------------
Please move this comparison of `getFeaturesStr()` against "default" into a
dedicated function on `TargetAttr` (`::isDefaultVersion()`?). This magic string
literal is appearing quite a lot in this patch...
================
Comment at: lib/Serialization/ASTReaderDecl.cpp:2818
}
+ // Multiversioned functions with different
+ if (FuncX->isMultiVersion()) {
----------------
This comment is missing a
================
Comment at: lib/Serialization/ASTReaderDecl.cpp:2822
+ const auto *TAY = FuncY->getAttr<TargetAttr>();
+ assert(TAX && TAY && "Multiversion Function without target attribute");
+
----------------
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.
================
Comment at: lib/Serialization/ASTReaderDecl.cpp:2824
+
+ if(TAX->getFeaturesStr() != TAY->getFeaturesStr())
+ return false;
----------------
Space after `if`, please.
================
Comment at: test/CodeGen/attr-target-mv-func-ptrs.c:10-15
+int bar() {
+ func(foo);
+ FuncPtr Free = &foo;
+ FuncPtr Free2 = foo;
+ return Free(1) + Free(2);
+}
----------------
What about uses in contexts where there is no target function type? For
example, `+foo;`
================
Comment at: test/CodeGenCXX/attr-target-mv-member-funcs.cpp:3-8
+struct S {
+ int __attribute__((target("sse4.2"))) foo(int) { return 0; }
+ int __attribute__((target("arch=sandybridge"))) foo(int);
+ int __attribute__((target("arch=ivybridge"))) foo(int) { return 1; }
+ int __attribute__((target("default"))) foo(int) { return 2; }
+};
----------------
OK, multiversioned member functions! Let's look at some nasty corner cases!
Do you allow multiversioning of special member functions (copy constructor,
destructor, ...)? Some tests for that would be interesting. Note in particular
that `CXXRecordDecl::getDestructor` assumes that there is only one destructor
for a class, and I expect we make that assumption in a bunch of other places
too. Might be best to disallow multiversioning destructors for now.
Do you allow a multiversioned function to have one defaulted version and one
non-defaulted version? What does that mean for the properties of the class that
depend on whether special member functions are trivial? Might be a good idea to
disallow defaulting a multiversioned function. Oh, and we should probably not
permit versions of a multiversioned function to be deleted either.
If you allow multiversioning of constructors, I'd like to see a test for
multiversioning of inherited constructors. Likewise, if you allow
multiversioning of conversion functions, I'd like to see a test for that. (I
actually think there's a very good chance both of those will just work fine.)
You don't allow multiversioning of function templates right now, but what about
multiversioning of member functions of a class template? Does that work? If so,
does class template argument deduction using multiversioned constructors work?
Does befriending multiversioned functions work? Is the target attribute taken
into account?
https://reviews.llvm.org/D40819
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
