Try to implement lambdas with inalloca parameters by inlining the call operator function.

[nlopes]

clang/lib/CodeGen/CGClass.cpp

3080

Please use PoisonValue instead whenever possible, as we are trying to remove undef from LLVM.
Thank you!

[akhuang]

update

[akhuang]

update

[akhuang]

update

[rnk]

clang/lib/CodeGen/CGClass.cpp

3064

Let's use getNullValue instead, just to avoid any complications for msan or ubsan, which may check for defined-ness.

clang/lib/CodeGen/CodeGenModule.cpp

3588

Can you move most of the details of this out of line to try to keep this as high level as possible?

3590–3593

For simplicity, what if we always emitted the call operator for all lambda static invokers into the IR first? So, this logic would then become almost exactly the same as the emitCXXStructor logic above.

Later, in EmitLambdaStaticInvokeBody, we can detect the inalloca case and start the cloning.

[akhuang]

clang/lib/CodeGen/CodeGenModule.cpp

3590–3593

For simplicity, what if we always emitted the call operator for all lambda static invokers into the IR first? So, this logic would then become almost exactly the same as the emitCXXStructor logic above.

Do you know where this should happen? I couldn't really figure out a place other than here for emitting the call operator.

If I do the cloning inside the normal EmitLambdaStaticInvokeBody path it's a bit annoying because StartFunction/EndFunction get called before/after cloning.

[rnk]

clang/lib/CodeGen/CodeGenModule.cpp

3590–3593

Do you know where this should happen? I couldn't really figure out a place other than here for emitting the call operator.

Yes, I think you should do that here, just like we do for constructors. If it's a hack, it's one we already have. The main impact is that we emit the call operator in the IR first. That may require updating some tests, but it's nice to do the same thing on all platforms, and we'd need to do it to handle forwarding varargs lambdas anyway, which are present on all platforms.

I also think it's OK to delete all the IR from StartFunction after its been generated, that doesn't seem like a big deal. How does the varargs cloning logic handle this situation?

[akhuang]

clang/lib/CodeGen/CodeGenModule.cpp

3590–3593

Oh, ok, I see what you mean.

I'll try to upload a version with the cloning function inside EmitLambdaStaticInvokeBody. I think there's some stuff in Start/End Function that prevent you from simply deleting the code. (I don't think it's an issue for the varargs cloning because that isn't called within EmitGlobalFunctionDefinition. )

[akhuang]

clang/lib/CodeGen/CodeGenModule.cpp

3590–3593

Actually, hm, trying to do the function cloning inside EmitLambdaStaticInvokeBody/GenerateCode might not work because FinishFunction does various things like emit the return statement, which won't work if we just cloned the function.

[akhuang]

moved some stuff around

[rnk]

+@efriedma, can you review this as a Clang CodeGen owner, according to the recently updated CodeOwners.rst file?
https://github.com/llvm/llvm-project/blob/main/clang/CodeOwners.rst#clang-llvm-ir-generation

clang/lib/CodeGen/CodeGenModule.cpp

3590–3593

If it doesn't work at all, I'm OK with doing this here, but I would like to try to move as much logic as possible out of EmitGlobalDefinition.

[akhuang]

clang/lib/CodeGen/CodeGenModule.cpp

3590–3593

Yeah, it doesn't seem ideal.. I guess the logic can also be moved into EmitGlobalFunctionDefinition (or a new CodeGenModule wrapper function) but that doesn't seem much better.

I don't think cloning the function in GenerateCode between StartFunction and FinishFunction will work though. Otherwise maybe can go back to trying a different method like changing the CXXMethodDecl from the call op and emitting it through the normal codegen path.

[efriedma]

Should we try to use this codepath for variadic lambdas as well?

Do we want to try to unify our cloning code? CodeGenFunction::GenerateVarArgsThunk has code doing something similar. (It's at least worth comparing to see if you're doing something significantly different...)

clang/lib/CodeGen/CGClass.cpp

3063

Checking the name doesn't work; -fdiscard-value-names is on by default in Release builds

[efriedma]

Might also be worth considering if we can avoid cloning here. It should be possible to emit the lambda body into a separate function with a calling convention of your choice, and make both the call operator and the static invoker call it.

[akhuang]

Move cloning code into a function.

[akhuang]

In D136998#3906881, @efriedma wrote:

Might also be worth considering if we can avoid cloning here. It should be possible to emit the lambda body into a separate function with a calling convention of your choice, and make both the call operator and the static invoker call it.

Thanks for reviewing! I started a patch to emit a new function with a different calling convention here https://reviews.llvm.org/D137872. It probably has some issues - I'm not sure if the argument arranging for the call actually works generally, and it has a bunch of copied code from EmitCall and EmitLambdaForwardingCall. But, it doesn't involve changing code in EmitGlobalDefinition, which seems better.

[akhuang]

Fix calling convention of cloned function.

[rnk]

In D136998#3906874, @efriedma wrote:

Should we try to use this codepath for variadic lambdas as well?

Yes!

Do we want to try to unify our cloning code? CodeGenFunction::GenerateVarArgsThunk has code doing something similar. (It's at least worth comparing to see if you're doing something significantly different...)

Good idea

In D136998#3906881, @efriedma wrote:

Might also be worth considering if we can avoid cloning here. It should be possible to emit the lambda body into a separate function with a calling convention of your choice, and make both the call operator and the static invoker call it.

This would be nice. I wasn't able to provide guidance on how to do that, and I think Amy was struggling to synthesize a new method (__invoke, __impl) at the AST layer.

[efriedma]

In D136998#3926321, @rnk wrote:

In D136998#3906874, @efriedma wrote:

Should we try to use this codepath for variadic lambdas as well?

Yes!

Do we want to try to unify our cloning code? CodeGenFunction::GenerateVarArgsThunk has code doing something similar. (It's at least worth comparing to see if you're doing something significantly different...)

Good idea

In D136998#3906881, @efriedma wrote:

Might also be worth considering if we can avoid cloning here. It should be possible to emit the lambda body into a separate function with a calling convention of your choice, and make both the call operator and the static invoker call it.

This would be nice. I wasn't able to provide guidance on how to do that, and I think Amy was struggling to synthesize a new method (__invoke, __impl) at the AST layer.

I think you wouldn't actually synthesize it at all in the AST.

Basically, the follow code changes:

1. Change the mangling of the function that contains the actual lambda body.
2. Make that function use an alternate calling convention that doesn't involve inalloca etc.
3. Synthesize thunks to represent the actual call operator and static invoker.

This is similar to the way virtual overriding works: there's an actual function, but there are also alternate entry points that end up in the vtables.

[akhuang]

In D136998#3926368, @efriedma wrote:

In D136998#3926321, @rnk wrote:

In D136998#3906874, @efriedma wrote:

Should we try to use this codepath for variadic lambdas as well?

Yes!

Do we want to try to unify our cloning code? CodeGenFunction::GenerateVarArgsThunk has code doing something similar. (It's at least worth comparing to see if you're doing something significantly different...)

Good idea

In D136998#3906881, @efriedma wrote:

Might also be worth considering if we can avoid cloning here. It should be possible to emit the lambda body into a separate function with a calling convention of your choice, and make both the call operator and the static invoker call it.

This would be nice. I wasn't able to provide guidance on how to do that, and I think Amy was struggling to synthesize a new method (__invoke, __impl) at the AST layer.

I think you wouldn't actually synthesize it at all in the AST.

Basically, the follow code changes:

1. Change the mangling of the function that contains the actual lambda body.
2. Make that function use an alternate calling convention that doesn't involve inalloca etc.
3. Synthesize thunks to represent the actual call operator and static invoker.

This is similar to the way virtual overriding works: there's an actual function, but there are also alternate entry points that end up in the vtables.

Thanks, that makes sense. I'm not very familiar with thunks but I'll look into it. Would we make a thunk that contains the call to the new lambda body function, and then the invoker and call op functions return that thunk?

clang/lib/CodeGen/CGClass.cpp

 #include "clang/AST/CharUnits.h"
 #include "clang/AST/DeclTemplate.h"
 #include "clang/AST/EvaluatedExprVisitor.h"
 #include "clang/AST/RecordLayout.h"
 #include "clang/AST/StmtCXX.h"
 #include "clang/Basic/CodeGenOptions.h"
 #include "clang/Basic/TargetBuiltins.h"
 #include "clang/CodeGen/CGFunctionInfo.h"
+#include "llvm/IR/Instructions.h"
 #include "llvm/IR/Intrinsics.h"
 #include "llvm/IR/Metadata.h"
+#include "llvm/Transforms/Utils/Cloning.h"
 #include "llvm/Transforms/Utils/SanitizerStats.h"
 
 using namespace clang;
 using namespace CodeGen;
 
 /// Return the best known alignment for an unknown pointer to a
 /// particular class.
 CharUnits CodeGenModule::getClassPointerAlignment(const CXXRecordDecl *RD) {
if (MD->isVariadic()) {
     // FIXME: Making this work correctly is nasty because it requires either
     // cloning the body of the call operator or making the call operator forward.
     CGM.ErrorUnsupported(MD, "lambda conversion to variadic function");
     return;
   }
 
   EmitLambdaDelegatingInvokeBody(MD);
 }
+
+void CodeGenFunction::EmitClonedLambdaStaticInvoke(const CXXMethodDecl *MD) {
+  const CGFunctionInfo &InvokerFnInfo = CGM.getTypes().arrangeCXXMethodDeclaration(MD);
+  llvm::Type *InvokerTy = CGM.getTypes().GetFunctionType(InvokerFnInfo);
+  GlobalDecl GD = GlobalDecl(MD);
+  llvm::Function *InvokerFn = cast<llvm::Function>(CGM.GetAddrOfFunction(GD, InvokerTy));
+
+  const CXXRecordDecl *Lambda = MD->getParent();
+  const CXXMethodDecl *CallOp = Lambda->getLambdaCallOperator();
+  const CGFunctionInfo &CallOpFnInfo =
+    CGM.getTypes().arrangeCXXMethodDeclaration(CallOp);
+  llvm::Function *CallOpFn = cast<llvm::Function>(
+    CGM.GetAddrOfFunction(GlobalDecl(CallOp),
+                          CGM.getTypes().GetFunctionType(CallOpFnInfo)));
+
+  // Generate code for call op.
+  if (CallOpFn->isDeclaration()) {
+    CodeGenFunction(CGM).GenerateCode(GlobalDecl(CallOp), CallOpFn, CallOpFnInfo);
+  }
+
+  // Clone from call operator.
+  llvm::ValueToValueMapTy VMap;
+
+  // Don't copy the %this argument.
+  // Assume CallOp is going to have two parameters?: this and inalloca.
+  for (llvm::Argument &I : CallOpFn->args()) {
+    if (I.getName().equals("this")) {
+      VMap[&I] = llvm::PoisonValue::get(I.getType());
+    } else {
+      for (llvm::Argument &DestI : InvokerFn->args()) {
+        if (DestI.getType() == I.getType()) {
+          VMap[&I] = &DestI;
+          break;
+        }
+      }
+    }
+  }
+
+  InvokerFn->dropAllReferences(); // Erase function  body.
+  SmallVector<llvm::ReturnInst *, 8> Returns;
+  llvm::CloneFunctionInto(InvokerFn, CallOpFn, VMap, 
+      llvm::CloneFunctionChangeType::LocalChangesOnly, Returns, "", nullptr);
+}

clang/lib/CodeGen/CodeGenFunction.h

public:
   void EmitFunctionBody(const Stmt *Body);
   void EmitBlockWithFallThrough(llvm::BasicBlock *BB, const Stmt *S);
 
   void EmitForwardingCallToLambda(const CXXMethodDecl *LambdaCallOperator,
                                   CallArgList &CallArgs);
   void EmitLambdaBlockInvokeBody();
   void EmitLambdaDelegatingInvokeBody(const CXXMethodDecl *MD);
   void EmitLambdaStaticInvokeBody(const CXXMethodDecl *MD);
+  void EmitClonedLambdaStaticInvoke(const CXXMethodDecl *MD);
+
   void EmitLambdaVLACapture(const VariableArrayType *VAT, LValue LV) {
     EmitStoreThroughLValue(RValue::get(VLASizeMap[VAT->getSizeExpr()]), LV);
   }
   void EmitAsanPrologueOrEpilogue(bool Prologue);
 
   /// Emit the unified return block, trying to avoid its emission when
   /// possible.
   /// \return The debug location of the user written return statement if the

clang/lib/CodeGen/CodeGenModule.cpp

for (unsigned I = 0; I < Clone->featuresStrs_size(); ++I)
       if (Clone->isFirstOfVersion(I))
         EmitGlobalFunctionDefinition(GD.getWithMultiVersionIndex(I), nullptr);
     // Ensure that the resolver function is also emitted.
     GetOrCreateMultiVersionResolver(GD);
   } else
     EmitGlobalFunctionDefinition(GD, GV);
 }
 
+static bool isInAllocaArgument(CGCXXABI &ABI, QualType T) {
+  const CXXRecordDecl *RD = T->getAsCXXRecordDecl();
+  return RD && ABI.getRecordArgABI(RD) == CGCXXABI::RAA_DirectInMemory;
+}
+
 void CodeGenModule::EmitGlobalDefinition(GlobalDecl GD, llvm::GlobalValue *GV) {
   const auto *D = cast<ValueDecl>(GD.getDecl());
 
   PrettyStackTraceDecl CrashInfo(const_cast<ValueDecl *>(D), D->getLocation(),
                                  Context.getSourceManager(),
                                  "Generating code for declaration");
 
   if (const auto *FD = dyn_cast<FunctionDecl>(D)) {
llvm::TimeTraceScope TimeScope("CodeGen Function", [&]() {
       return Name;
     });
 
     if (const auto *Method = dyn_cast<CXXMethodDecl>(D)) {
       // Make sure to emit the definition(s) before we emit the thunks.
       // This is necessary for the generation of certain thunks.
       if (isa<CXXConstructorDecl>(Method) || isa<CXXDestructorDecl>(Method))
         ABI->emitCXXStructor(GD);
+      // Hack for lambda forwarding calls with inalloca parameters.
+      else if (isa<CXXMethodDecl>(FD) && cast<CXXMethodDecl>(FD)->isLambdaStaticInvoker() &&
+          getTarget().getCXXABI().isMicrosoft() &&
+          llvm::any_of(cast<CXXMethodDecl>(FD)->parameters(), [&](ParmVarDecl *P) {
+            return isInAllocaArgument(getCXXABI(), P->getType());
+            })) {
+        // If the call op is not yet emitted, emit that.
+        const CXXRecordDecl *Lambda = cast<CXXMethodDecl>(FD)->getParent();
+        const CXXMethodDecl *CallOp = Lambda->getLambdaCallOperator();
+        const CGFunctionInfo &CallOpFnInfo =
+          getTypes().arrangeCXXMethodDeclaration(CallOp);
+        llvm::Function *CallOpFn = cast<llvm::Function>(
+          GetAddrOfFunction(GlobalDecl(CallOp),
+                            getTypes().GetFunctionType(CallOpFnInfo)));
+        if (CallOpFn->isDeclaration())
+          EmitGlobalFunctionDefinition(GlobalDecl(CallOp), nullptr);
+
+        CodeGenFunction(*this).EmitClonedLambdaStaticInvoke(cast<CXXMethodDecl>(FD));
+      }
       else if (FD->isMultiVersion())
         EmitMultiVersionFunctionDefinition(GD, GV);
       else
         EmitGlobalFunctionDefinition(GD, GV);
 
       if (Method->isVirtual())
         getVTables().EmitThunks(GD);
 

clang/test/CodeGenCXX/inalloca-lambda.cpp

-// RUN: not %clang_cc1 -triple i686-windows-msvc -emit-llvm -o /dev/null %s  2>&1 | FileCheck %s
-
-// PR28299
-// CHECK: error: cannot compile this forwarded non-trivially copyable parameter yet
+// RUN: %clang_cc1 -triple i686-windows-msvc -emit-llvm -o - %s  2>&1 | FileCheck %s
 
 class A {
   A(const A &);
 };
 typedef void (*fptr_t)(A);
 fptr_t fn1() { return [](A) {}; }
 
+// CHECK: define internal x86_thiscallcc void @"?__invoke@<lambda_0>@?0??fn1@@YAP6AXVA@@@ZXZ@CA?A?<auto>@@0@Z"
+// CHECK-SAME: (ptr inalloca(<{ %class.A, [3 x i8] }>) %0)
+// CHECK: %1 = getelementptr inbounds <{ %class.A, [3 x i8] }>, ptr %0, i32 0, i32 0
+// CHECK: ret void