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