This is an archive of the discontinued LLVM Phabricator instance.

Reland "Try to implement lambdas with inalloca parameters by forwarding without use of inallocas."
ClosedPublic

Authored by akhuang on Jun 28 2023, 2:17 PM.

Details

Summary

Original commit:
Lambda functions with inalloca parameters (which are used in 32-bit Windows) currently aren't supported because the call operator delegates to another function and forwards its arguments, and inalloca parameters can't be forwarded. (The same issue exists for variadic arguments.)

This patch emits the lambda call operator again with a different calling convention without inallocas. Then the original call operator and static invoker delegate to this new function. We also modify arrangeLLVMFunctionInfo to be able to generate a different CGFunctionInfo for delegate calls that doesn't use inalloca.

Fixes https://github.com/llvm/llvm-project/issues/28673.

Changes:

  • Make sure we only emit the new callop function body if the static invoker is actually used
  • Hopefully avoid making a copy of the forward types when calling the impl function.

This reverts commit 8ed7aa59f489715d39d32e72a787b8e75cfda151.

Diff Detail

Event Timeline

akhuang created this revision.Jun 28 2023, 2:17 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 28 2023, 2:17 PM
akhuang requested review of this revision.Jun 28 2023, 2:17 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 28 2023, 2:17 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript
akhuang edited the summary of this revision. (Show Details)Jun 28 2023, 2:27 PM

I'm not confident that isUsed() works the way you want it to in this context. In particular, if the code in question runs before we've translated the whole translation unit, the isUsed() bit could change. If you want that's more obviously safe, you could just check if there are any captures. (I'm assuming the point of this is just to reduce the number of lambdas that go through this codepath?)

rnk added a comment.Jun 28 2023, 3:50 PM

Can you please add a test case for the issue that caused the revert? I wanted to dig into that to try to understand why the extra copy was being emitted. I think you mentioned it has something to do with increasing the alignment.

I'm not confident that isUsed() works the way you want it to in this context. In particular, if the code in question runs before we've translated the whole translation unit, the isUsed() bit could change. If you want that's more obviously safe, you could just check if there are any captures. (I'm assuming the point of this is just to reduce the number of lambdas that go through this codepath?)

Oh, thanks. Yeah, the point is we don't have to go down this path if we never have to emit the invoker.

Can you please add a test case for the issue that caused the revert? I wanted to dig into that to try to understand why the extra copy was being emitted. I think you mentioned it has something to do with increasing the alignment.

oops, I added comments when I put up the patch but never submitted them.. there's a repro in crbug.com/1457256 but I'll also add a test case

clang/lib/CodeGen/CGCall.cpp
5103–5105

I think the problem is that it tries to do a copy here because the alignment of the forwarding function arg is larger than the alignment of the object that's being passed. I'm not sure how alignments are computed or if there are any other requirements for alignment. Is it ok to just ignore the new alignment? Do we need to change the code that computes the argument alignment?

(crbug.com/1457256#comment2 has an example repro)

rnk added inline comments.Jun 28 2023, 4:06 PM
clang/lib/CodeGen/CGCall.cpp
5103–5105

Yes, in general, structs with doubles and i64 members are passed misaligned on i686. This is true for all functions, not just lambdas. We should power down whatever alignment logic is causing the copy.

akhuang added inline comments.Jun 28 2023, 4:13 PM
clang/lib/CodeGen/CGCall.cpp
5103–5105

huh, ok, good to know.

akhuang added inline comments.Jul 12 2023, 4:11 PM
clang/lib/CodeGen/CodeGenFunction.cpp
1319

I'm not confident that isUsed() works the way you want it to in this context. In particular, if the code in question runs before we've translated the whole translation unit, the isUsed() bit could change. If you want that's more obviously safe, you could just check if there are any captures. (I'm assuming the point of this is just to reduce the number of lambdas that go through this codepath?)

Oh, checking for captures would be the same as checking for getLambdaStaticInvoker(), right? I think I'll just keep with the same check from the original patch, then.

akhuang updated this revision to Diff 539784.Jul 12 2023, 4:11 PM

Fix alignment problem

rnk added inline comments.Jul 12 2023, 4:23 PM
clang/lib/CodeGen/CGCall.cpp
5104

Please add a comment about this. We need to avoid copying uncopyable objects passed in misaligned inalloca argument packs.

Another approach we could take here is to check CXXRecordDecl::isTriviallyCopyable, and avoid doing the copy for such argument types. That would be non-conforming anyway, and results in an assert in CallArg::copyInto. For trivially copyable types, doing the copy may actually be good, since it's more conforming.

akhuang added inline comments.Jul 12 2023, 4:40 PM
clang/lib/CodeGen/CGCall.cpp
5104

Sorry, didn't mean to include this in the patch.

akhuang updated this revision to Diff 539795.Jul 12 2023, 4:41 PM

minor fixes

akhuang updated this revision to Diff 542721.Jul 20 2023, 4:55 PM

Change impl function naming scheme

akhuang added inline comments.Jul 20 2023, 5:03 PM
clang/lib/CodeGen/CGClass.cpp
3097

The old code tried to find the "<lambda_0>" part of the function name and replace the front with "?__impl@", so that the function name was consistent with the mangling scheme. But apparently there are some cases where when the function name is too long, it uses a hash instead (?), so the attempt to find the <lambda_0> was failing.

I couldn't find a good way to name this function through the actual mangling code -- is it fine to just add "__impl" to the front and have it not be mangled correctly?

akhuang updated this revision to Diff 543038.Jul 21 2023, 1:21 PM

another fix to function naming code (do the same thing as before except when the mangled name is a hash)

rnk added inline comments.Jul 24 2023, 12:41 PM
clang/lib/CodeGen/CGClass.cpp
3097

Let me suggest one more thing to try here to convince the mangler to do our work for us. You can create a CXXMethodDecl AST node here in CodeGen, and ask the mangler to mangle it for you. If you search CodeGen, you can find other examples of this, notably, for making implicit this variable declarations:

$ git grep Decl::Create'(' ../clang/lib/CodeGen/
../clang/lib/CodeGen/CGBuiltin.cpp:  Args.push_back(ImplicitParamDecl::Create(
../clang/lib/CodeGen/CGBuiltin.cpp:    Args.push_back(ImplicitParamDecl::Create(
../clang/lib/CodeGen/CGCXXABI.cpp:  auto *ThisDecl = ImplicitParamDecl::Create(

Call CXXMethodDecl::Create, and copy in all of the attribute from the call operator, but replace the DeclarationNameInfo with an IdentifierInfo of __impl.

If that's not working out, let's land this as is and follow up on fixing the mangling.

akhuang updated this revision to Diff 544086.Jul 25 2023, 1:32 PM

Add some functions to MicrosoftMangle so we can pass in a custom function name.

clang/lib/CodeGen/CGClass.cpp
3097

I think CXXMethodDecl::Create doesn't work because it needs a non-const CXXRecordDecl to construct?

I added some functions to MicrosoftMangler so that we can pass in a custom DeclarationName, does that seem ok?

rnk added inline comments.Jul 25 2023, 1:46 PM
clang/lib/CodeGen/CGClass.cpp
3097

I think we should go back to the last approach you had with the string substution, and then we can start a new code review where we consider the two alternative approaches. I see lots of examples of const_cast in the codebase, but maybe the custom name approach is better. Hard to say. Either way, it will be easier to resolve the question in a smaller code review. Sorry for the back and forth.

akhuang updated this revision to Diff 544090.Jul 25 2023, 1:54 PM

go back to previous __impl naming method

rnk accepted this revision.Jul 25 2023, 1:59 PM

lgtm

clang/lib/CodeGen/CGClass.cpp
3096

Please add a TODO comment, suggestion:
// TODO: Use the name mangler to produce the right name instead of using string replacement.

This revision is now accepted and ready to land.Jul 25 2023, 1:59 PM
This revision was landed with ongoing or failed builds.Jul 26 2023, 4:14 PM
This revision was automatically updated to reflect the committed changes.