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@.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@Z.resolver"(i32) comdat +// WINDOWS: call i32 @"?foo_overload@@YAHH@Z.arch_sandybridge" +// WINDOWS: call i32 @"?foo_overload@@YAHH@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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits