[PATCH] D38596: Implement attribute target multiversioning

[Erich Keane via Phabricator via cfe-commits]

erichkeane marked 31 inline comments as done.
erichkeane added a subscriber: rnk.
erichkeane added a comment.

Weew... I think I got everything.  Thanks for the review you three!  I've got another patch coming momentarily with what I believe is all your suggestions.



================
Comment at: include/clang/Basic/DiagnosticSemaKinds.td:9301
+def err_target_causes_illegal_multiversioning
+    : Error<"function redeclaration causes a multiversioned function, but a "
+            "previous declaration lacks a 'target' attribute">;
----------------
aaron.ballman wrote:
> 'causes' seems a bit strange; how about `function redeclaration declares a multiversioned function,`? (Or something else, I'm not tied to my wording.)
Thats as good as anything I can think of. I'll change to yours for now, but if someone else comes up with something that sounds better, I'll consider us open to it.


================
Comment at: lib/Basic/Targets/X86.cpp:1295
       .Case("amdfam10h", true)
+      .Case("amdfam10", true)
       .Case("amdfam15h", true)
----------------
hfinkel wrote:
> Can you please separate out these changes adding the amdfam10 target strings into a separate patch?
Talked to Craig on this... apparently the rules for amdfam10/amdfam15/amdfam10h/amdfam15h are a bit more complicated (-march supports ONLY the amdfam10, validateCpuIs only supports the "h" versions (of both), and only the 'march' version is supported in this feature.  I may have to toss another function to mess with these strings to make it work.


================
Comment at: lib/CodeGen/CodeGenFunction.cpp:2279
+llvm::Value *
+CodeGenFunction::FormResolverCondition(const ResolverOption &RO) {
+  llvm::Value *TrueCondition = nullptr;
----------------
There IS a "CodeGen/TargetInfo" here, but it isn't as comprehensive as the Basic/TargetInfo types.  I wonder if there would be value in creating a 'TargetCodeGenInfo::EmitCpuIs' and TargetCodeGenInfo::EmitCpuSupports to replace the X86 version of these, and suppressing this?

It might be valuable to simply replace the 'EmitTargetArchBuiltinExpr' with something to do this as well.  Those handful of target-arch-builtins are a little messy, and are pretty messy in CGFunction.

Probably will have to ask @rnk if he's got a good thought here.  I mentioned trying to break up this target info at one point, and he thought it wasn't a great idea.


================
Comment at: lib/CodeGen/CodeGenFunction.h:3707
+    llvm::Function *Function;
+    ResolverOption(TargetAttr::ParsedTargetAttr &&AttrInfo,// can be moved?
+                   llvm::Function *Function)
----------------
aaron.ballman wrote:
> The comment doesn't inspire confidence with the trailing question mark. ;-)
Woops!  Personal review comment was left in :)


================
Comment at: lib/Sema/SemaDecl.cpp:9297-9300
+    if (NewFD->hasAttr<TargetAttr>())
+      NewFD->setMultiVersionKind(FunctionDecl::MultiVersionKind::All);
+    else
+      NewFD->setMultiVersionKind(FunctionDecl::MultiVersionKind::None);
----------------
aaron.ballman wrote:
> Might be more succinctly written with `?:` (unsure which looks better, try it and see).
It does actually... Clang format makes this look really nice.


================
Comment at: lib/Sema/SemaDecl.cpp:9306
+  switch (OldFD->getMultiVersionKind()) {
+  default:
+    llvm_unreachable("Invalid State for Multiversioning.");
----------------
aaron.ballman wrote:
> I suspect this will give warnings about having a `default` label in a fully-covered switch.
I didn't see any, but I have no problem removing it anyway.


================
Comment at: test/CodeGenCXX/attr-target-multiversion.cpp:20
+}
+
+// CHECK: define i{{[0-9]+}} (%struct.S*)* @_ZN1S2mvEv.resolver
----------------
hfinkel wrote:
> Tests for interactions with function pointers and virtual functions?
Looking into it, I'm not sure how virtual functions should LOOK in this case (and GCC doesn't actually implement it either!).  I might punt for now while the rest of the patch is in shape, and add it in with function templates/etc.


https://reviews.llvm.org/D38596