rnk added inline comments.
================ Comment at: clang/include/clang/CodeGen/CGFunctionInfo.h:571 + /// Whether this function should avoid inalloca arguments. + unsigned DontInAlloca: 1; + ---------------- This is an amusing name, but we should try to find something better. :) Can we build on this flag to support re-writing a varargs prototype to receive a va_list? If so, we should plan for that, and name this to cover both use cases. Elsewhere in clang we use the terminology "delegate call" see [EmitDelegateCallArg](https://github.com/llvm/llvm-project/blob/2e999b7dd1934a44d38c3a753460f1e5a217e9a5/clang/lib/CodeGen/CGVTables.cpp#L341). Could this be something like `ForDelegation` or `ForDelegateCall` or `IsDelegateCall` or `IsDelegateTarget` or something like that? Or Thunk? I think I like `ForDelegateCall`, but I'm not sure. --- Does anyone know what a "chain call" is? It wasn't clear to me immediately. ================ Comment at: clang/lib/CodeGen/CGCall.cpp:190 - return CGT.arrangeLLVMFunctionInfo(resultType, instanceMethod, - /*chainCall=*/false, prefix, - FTP->getExtInfo(), paramInfos, - Required); + FnInfoOptions opts(instanceMethod, /*IsChainCall*/false, /*DontInAlloca*/false); + return CGT.arrangeLLVMFunctionInfo(resultType, opts, prefix, FTP->getExtInfo(), ---------------- This still requires boolean parameters in order. I think LLVM tends to use flag enums for this, something like: ``` enum class FnInfoOptions { None = 0, IsInstanceMethod = 1 << 0, IsChainCall = 1 << 1, IsDelegateCall = 1 << 2, }; ``` Call sites look like: ``` arrangeLLVMFunctionInfo(RetTy, FnInfoOptions::None, None, FTNP->getExtInfo()...) arrangeLLVMFunctionInfo(RetTy, FnInfoOptions::IsInstanceMethod, None, FTNP->getExtInfo()...) ``` ================ Comment at: clang/lib/CodeGen/CGClass.cpp:3015 +static bool hasInAllocaArg(const TargetInfo &TI, CGCXXABI &ABI, const CXXMethodDecl *MD) { + return TI.getCXXABI().isMicrosoft() && + llvm::any_of(MD->parameters(), [&](ParmVarDecl *P) { ---------------- I believe this is only necessary for x86 platforms, so we should check the architecture too to avoid the extra work on x64. ================ Comment at: clang/lib/CodeGen/CGClass.cpp:3097 + + std::string ImplName = (CallOpFn->getName() + ".impl").str(); + llvm::Function *Fn = CallOpFn->getParent()->getFunction(ImplName); ---------------- We should use a name which demangles correctly, as we do for `__invoke`. This is a good first draft, though. ================ Comment at: clang/test/CodeGenCXX/inalloca-lambda.cpp:4 class A { A(const A &); }; ---------------- Just to make the IR slightly nicer, give this an `int` member. ================ Comment at: clang/test/CodeGenCXX/inalloca-lambda.cpp:8 fptr_t fn1() { return [](A) {}; } + ---------------- Please expand the testing further to include a case where the same lambda is called directly and goes through function pointer decay, something like this: https://gcc.godbolt.org/z/q7x3ov5e1 This is to ensure that we don't end up double-emitting the implementation of the lambda. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D137872/new/ https://reviews.llvm.org/D137872 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
