Page MenuHomePhabricator

Implement lambdas with inalloca parameters by forwarding to function without inalloca calling convention.
Needs ReviewPublic

Authored by akhuang on Nov 11 2022, 3:19 PM.

Details

Reviewers
rnk
efriedma
Summary

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.

Diff Detail

Unit TestsFailed

TimeTest
110 msx64 debian > LLVM.Examples/OrcV2Examples::lljit-with-thinlto-summaries.test
Script: -- : 'RUN: at line 1'; /var/lib/buildkite-agent/builds/llvm-project/build/bin/opt -module-summary /var/lib/buildkite-agent/builds/llvm-project/llvm/test/Examples/OrcV2Examples/Inputs/main-mod.ll -o /var/lib/buildkite-agent/builds/llvm-project/build/test/Examples/OrcV2Examples/Output/main-mod.bc
1,750 msx64 debian > libFuzzer.libFuzzer::fuzzer-finalstats.test
Script: -- : 'RUN: at line 1'; /var/lib/buildkite-agent/builds/llvm-project/build/./bin/clang --driver-mode=g++ -O2 -gline-tables-only -fsanitize=address,fuzzer -I/var/lib/buildkite-agent/builds/llvm-project/compiler-rt/lib/fuzzer -m64 /var/lib/buildkite-agent/builds/llvm-project/compiler-rt/test/fuzzer/SimpleTest.cpp -o /var/lib/buildkite-agent/builds/llvm-project/build/projects/compiler-rt/test/fuzzer/X86_64DefaultLinuxConfig/Output/fuzzer-finalstats.test.tmp-SimpleTest

Event Timeline

akhuang created this revision.Nov 11 2022, 3:19 PM
Herald added a project: Restricted Project. · View Herald TranscriptNov 11 2022, 3:19 PM
akhuang requested review of this revision.Nov 11 2022, 3:19 PM
Herald added a project: Restricted Project. · View Herald TranscriptNov 11 2022, 3:19 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript

I'm not quite sure I understand what's happening here. Does this actually avoid generating two copies of the function body if both the call operator and the conversion are used?

akhuang updated this revision to Diff 476633.Nov 18 2022, 4:29 PM

Clean up existing code and add code to make the call operator also call the new function.

I'm not quite sure I understand what's happening here. Does this actually avoid generating two copies of the function body if both the call operator and the conversion are used?

That was intended but missing; patch is now updated to make both the invoker and the call operator call the new function.

rnk added inline comments.Nov 21 2022, 11:52 AM
clang/include/clang/CodeGen/CGFunctionInfo.h
571

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. 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.

clang/lib/CodeGen/CGCall.cpp
190–193

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()...)
clang/lib/CodeGen/CGClass.cpp
3015

I believe this is only necessary for x86 platforms, so we should check the architecture too to avoid the extra work on x64.

3098

We should use a name which demangles correctly, as we do for __invoke. This is a good first draft, though.

clang/test/CodeGenCXX/inalloca-lambda.cpp
5–6

Just to make the IR slightly nicer, give this an int member.

36–50

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.

akhuang updated this revision to Diff 477289.Nov 22 2022, 1:52 PM
akhuang marked 4 inline comments as done.

add to test case, modify name mangling, change fn info opts enum type

rnk added inline comments.Nov 22 2022, 3:55 PM
clang/lib/CodeGen/CGCall.cpp
727

We could avoid static_cast by defining the standard flag operators, similar to what we do in this macro:
https://github.com/llvm/llvm-project/blob/main/llvm/include/llvm/DebugInfo/CodeView/CodeView.h#L52

Alternatively, just define operator| and operator&.

clang/lib/CodeGen/TargetInfo.cpp
1190

Please update the old parameter name

1841

As a simplification, you can put || isDelegateCall here, we're basically pretending that this returns RAA_Indirect.

akhuang updated this revision to Diff 478324.Nov 28 2022, 12:20 PM
akhuang marked 3 inline comments as done.

Address comments

akhuang retitled this revision from Try to implement lambdas with inalloca parameters by forwarding without use of inallocas. to Implement lambdas with inalloca parameters by forwarding to function without inalloca calling convention..Dec 5 2022, 2:47 PM
akhuang edited the summary of this revision. (Show Details)

ping @efriedma, do you mind looking at this again? thanks!

clang/include/clang/CodeGen/CGFunctionInfo.h
571

Yeah, that naming makes sense.

chain call: https://reviews.llvm.org/D6332?

clang/lib/CodeGen/CGClass.cpp
3098

Fixed to replace different part of the mangled name with __impl. Don't know if it would be better to use the actual mangling functions, but the string replace here seems simple enough.