[PATCH] D40819: Implement Attribute Target MultiVersioning (Improved edition!)

[Erich Keane via Phabricator via cfe-commits]

erichkeane marked 14 inline comments as done.
erichkeane added a comment.

Incoming patch!



================
Comment at: include/clang/Basic/Attr.td:1809
       bool DuplicateArchitecture = false;
+      bool operator ==(const ParsedTargetAttr &Other) {
+        return DuplicateArchitecture == Other.DuplicateArchitecture &&
----------------
rsmith wrote:
> Our normal convention is to not put a space before `==` here.
Clang format seems to keep adding it interestingly enough.  Perhaps because it is a .td file?  Will remove.


================
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 :
----------------
rsmith wrote:
> 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.
Thanks!  Clang-format really makes a mess of these too...


================
Comment at: lib/CodeGen/CodeGenModule.cpp:2144
+        if (getContext().hasSameType(CurFD->getType(), FD->getType())) {
+          StringRef MangledName = getMangledName(CurFD);
+          llvm::Constant *Func = GetGlobalValue(MangledName);
----------------
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.


================
Comment at: lib/Sema/SemaDecl.cpp:9326-9328
+static bool CheckMultiVersionAdditionalRules(Sema &S, const FunctionDecl *OldFD,
+                                             const FunctionDecl *NewFD,
+                                             bool CausesMV) {
----------------
rsmith wrote:
> 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).
I'd thought about that, but I'm being WAAY more strict, so I didn't see a good way to factor them out without making things pretty complicated.  Suggestions welcome :)


================
Comment at: lib/Sema/SemaDecl.cpp:9383-9384
+
+    QualType OldReturnType = OldType->getReturnType();
+    QualType NewReturnType = NewType->getReturnType();
+    if (OldReturnType != NewReturnType) {
----------------
rsmith wrote:
> 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?
I think disallowing 'auto' return for now is a good idea. This next patch is using isUndeducedType, which I think is sufficient, but if there is a better function I'm missing, please let me know.


https://reviews.llvm.org/D40819