[PATCH] D137872: Try to implement lambdas with inalloca parameters by forwarding without use of inallocas.

[Reid Kleckner via Phabricator via cfe-commits]

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