[PATCH] D53586: Implement Function Multiversioning for Non-ELF Systems.

[Reid Kleckner via Phabricator via cfe-commits]

rnk added a comment.

Seems reasonable. Should the resolver still be called `?foo at .resolver`, or should it get a new name, since it is quite functionally different now?



================
Comment at: lib/CodeGen/CodeGenFunction.cpp:2381
+
+template<typename BuilderTy>
+static void CreateMultiVersionResolverReturn(llvm::Function *Resolver,
----------------
Rather than templating over IRBuilder and CGBuilderTy, I'd standardize on CGBuilderTy.


================
Comment at: lib/CodeGen/CodeGenFunction.cpp:2391-2393
+  llvm::SmallVector<llvm::Value*, 10> Args;
+  llvm::for_each(Resolver->args(),
+                 [&](llvm::Argument &Arg) { Args.push_back(&Arg); });
----------------
Surely this would be simpler as a range for or `Args{Resolver->arg_begin() ...};`


================
Comment at: lib/CodeGen/CodeGenFunction.cpp:2395
+
+  llvm::CallInst *Result = Builder.CreateCall(FuncToReturn, Args);
+
----------------
This approach is... not going to work in the general case. Consider, for example, varargs. Then consider 32-bit x86 inalloca, which is quite widespread. Any non-trivially copyable object is not going to be passable through such a thunk. You might consider looking at CodeGenFunction::EmitMustTailThunk instead.


================
Comment at: lib/CodeGen/CodeGenFunction.cpp:2432
     llvm::BasicBlock *RetBlock = createBasicBlock("resolver_return", Resolver);
     llvm::IRBuilder<> RetBuilder(RetBlock);
+      CreateMultiVersionResolverReturn(Resolver, RetBuilder, RO.Function,
----------------
You can make this a CGBuilderTy. Just pass it a type cache.


================
Comment at: test/CodeGenCXX/attr-target-mv-overloads.cpp:74
+// WINDOWS: define dso_local i32 @"?foo_overload@@YAHH at Z.resolver"(i32) comdat
+// WINDOWS: call i32 @"?foo_overload@@YAHH at Z.arch_sandybridge"
+// WINDOWS: call i32 @"?foo_overload@@YAHH at Z.arch_ivybridge"
----------------
Does dumpbin demangle these names correctly? Just curious.


Repository:
  rC Clang

https://reviews.llvm.org/D53586