erichkeane marked 34 inline comments as done.
erichkeane added a comment.
Patch incoming, sorry it took so long!
================
Comment at: lib/CodeGen/CGBuiltin.cpp:7673
-Value *CodeGenFunction::EmitX86CpuInit() {
+Value *CodeGenFunction::EmitX86CpuInit(CGBuilderTy &Builder) {
llvm::FunctionType *FTy = llvm::FunctionType::get(VoidTy,
----------------
echristo wrote:
> Why do you need to pass in a Builder?
Because when I first wrote this, the EmitResolver function was a free function.
I guess I missed this dependency along the way, thanks for the catch!
================
Comment at: lib/CodeGen/CodeGenFunction.cpp:2324
+ llvm::Triple::x86_64) &&
+ "Only implemented for x86 targets");
+
----------------
echristo wrote:
> Can you get here via trying to compile code for another cpu?
At the moment, no. I'm asserting to make sure that is the case, and to be a
hint for George/et-al who are implementing this in the future for other
processors.
================
Comment at: lib/CodeGen/CodeGenModule.cpp:840-841
+
+ const auto *ND = cast<NamedDecl>(GD.getDecl());
+ UpdateMultiVersionNames(GD, ND);
+
----------------
rsmith wrote:
> 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?)
Unfortunately it is too late at that point. The problem is you have:
TARGET_SSE MVFunc();
TARGET_DEF MVFunc(); // At this point, a mangling-conflict occurs.
void foo() {
MVFunc(); // Only at THIS point does the IFunc get created, too late to rewrite
the SSE variant's name.
}
That said, I moved it to a more appropriate place.
================
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>())
----------------
rsmith wrote:
> 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.)
The actual case I encountered was that when emitting the global in
emitMultiVersionFunctions, the EmitGlobalDefinition was immediately skipping it
here because it was an inline function.
I believe the correct response is to just call EmitGlobalFunctionDefinition
from the emitMultiVersionFunctions handler. This will have to be modified when
I support virtual functions or constructors, but this will make it work.
IMO, the cross-TU issue you come up with would only be an issue when the value
isn't emitted due to it being inline/static/etc. In this case, the failed
linking is an acceptable consequence to the user improperly providing a
multiversion variant.
================
Comment at: lib/Sema/SemaDecl.cpp:9720
+ if (CheckMultiVersionFunction(*this, NewFD, Redeclaration, OldDecl,
+ MergeTypeWithPrevious, Previous))
+ return Redeclaration;
----------------
rsmith wrote:
> 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?
Looks like I'd just grabbed the wrong name for the parameter in
CheckMultiVersionFunction. Fixed!
================
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);
+}
----------------
rsmith wrote:
> What about uses in contexts where there is no target function type? For
> example, `+foo;`
Added as a test in Sema/attr-target-mv.c.
================
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; }
+};
----------------
rsmith wrote:
> 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?
I gave CTORs a try, and found that there are a few subtle issues that need to
be dealt with in code-gen. For the moment (and since GCC simply terminates if
you try to use them), I'd like to disallow them.
Definitely going to disallow dtors/default/deleted functions.
https://reviews.llvm.org/D40819
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
