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

[Erich Keane via Phabricator via cfe-commits]

erichkeane marked 2 inline comments as done.
erichkeane added a comment.

In https://reviews.llvm.org/D53586#1273546, @rnk wrote:

> 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?


I'm not attached to the name.  I didn't have a good alternative (I'd thought of 'dispatcher' at one point, but figured that was because my mind was attached to cpu-dispatch).  Implementation wise, '.resolver' is slightly easier, but not enough to motivate the discussion.

If you like '.dispatcher' instead of '.resolver', I can do that, otherwise a brainstorming session would be appreciated!



================
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); });
----------------
rnk wrote:
> Surely this would be simpler as a range for or `Args{Resolver->arg_begin() ...};`
Surely... Unfortunately the types don't match.  Resolver Args are an array of Arguments (so the iterator is a Arg*).

HOWEVER, "Args" here needs to be an array ref to Value*, so the iterator would have to be a Value** (not a Arg*/Value*).  The point of this small vector is to create an array of _pointers_ from the array of Arguments.


================
Comment at: lib/CodeGen/CodeGenFunction.cpp:2395
+
+  llvm::CallInst *Result = Builder.CreateCall(FuncToReturn, Args);
+
----------------
rnk wrote:
> 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.
Oh dear... 
I'm unfamiliar with EmitMustTailThunk, is it self explanatory?  Any chance you can expound?  Should I call/use that function, or copy out of it?


================
Comment at: lib/CodeGen/CodeGenFunction.cpp:2432
     llvm::BasicBlock *RetBlock = createBasicBlock("resolver_return", Resolver);
     llvm::IRBuilder<> RetBuilder(RetBlock);
+      CreateMultiVersionResolverReturn(Resolver, RetBuilder, RO.Function,
----------------
rnk wrote:
> You can make this a CGBuilderTy. Just pass it a type cache.
I'd tried that a while and could never find a type cache to use. I'd not realized until just now (searching more for usages) that CodeGenFunction is one!


================
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"
----------------
rnk wrote:
> Does dumpbin demangle these names correctly? Just curious.
I don't know, I'd suspect not.  However, demangler.com (not sure what it uses?) seems to demangle them ignoring everything after the '.'.


Repository:
  rC Clang

https://reviews.llvm.org/D53586