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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
