Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

JIT: Add support for frozen structs in Swift reverse pinvokes #100344

Merged
merged 56 commits into from
Apr 5, 2024

Conversation

jakobbotsch
Copy link
Member

@jakobbotsch jakobbotsch commented Mar 27, 2024

This adds the final support for frozen structs in UCO methods.

This passes 2500 auto generated tests locally on both macOS x64 and arm64. This PR includes only 100 tests.

Frozen struct parameters are handled via the new ABI representation added in #100138. When such a parameter exists we always allocate space for it on the local stack frame. The struct is then reassembled from its passed constituents as the first thing in the codegen.

One complication is that there can be an arbitrary amount of codegen to handle this reassembling. We cannot handle an arbitrary amount of codegen in the prolog, so the reassembling is handled in two places. First, since the amount of register passed data is limited, we handle those in the prolog (which frees them up to be used for other things). If some pieces were passed on the stack the JIT then ensures that there is a scratch BB and generates the code to reassemble the remaining parts as the first thing in the scratch BB.

Since Swift structs can be passed by reference in certain cases this PR also enables FEATURE_IMPLICIT_BYREFS for SysV x64 to handle those cases. Depending on the TP impact we can refine some of the ifdefs around this.

TODO:

  • Fix lvRefCnt() == 0 case

@jakobbotsch
Copy link
Member Author

/azp run runtime-coreclr jitstress, runtime-coreclr jitstressregs

Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@jakobbotsch
Copy link
Member Author

The testing passed, the failure is #99810. Need to fix the formatting failures though.

@jakobbotsch
Copy link
Member Author

cc @dotnet/jit-contrib PTAL @amanasifkhalid

I think this will need a few fixups after #100429 is merged.

Diffs. Enabling implicit byrefs on SysV x64 results in some large TP regressions with an MSVC compiled cross compiler, but the clang compiled one doesn't look bad. Also some TP improvements on other platforms from some of the optimization work I did around the implicit byrefs as a result.

Copy link
Member

@amanasifkhalid amanasifkhalid left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM assuming runtime/stress pipelines pass. Thanks!

case 1:
return TYP_UBYTE;
case 2:
return TYP_USHORT;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we use unsigned types for these ones?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We generally normalize up to at least 4 bytes on all loads, and zero extensions sometimes have smaller encodings than sign extensions on xarch (similar change was made in #86631).

src/coreclr/jit/codegencommon.cpp Show resolved Hide resolved
src/coreclr/jit/flowgraph.cpp Outdated Show resolved Hide resolved
src/coreclr/jit/lclvars.cpp Outdated Show resolved Hide resolved
@jakobbotsch
Copy link
Member Author

I'll kick off the stress pipelines again just to be safe after the merge of #100429.

@jakobbotsch
Copy link
Member Author

/azp run runtime-coreclr jitstress, runtime-coreclr jitstressregs, runtime-coreclr jitstress2-jitstressregs

Copy link

Azure Pipelines successfully started running 3 pipeline(s).

@jakobbotsch
Copy link
Member Author

Also cc @jkoritzinsky, @jkurdek, @matouskozak, @kotlarmilos. With this PR I think we'll be feature complete on the Swift support in RyuJIT (until #100543 makes it through review, although I expect it will be straightforward to add support for those).

@jakobbotsch
Copy link
Member Author

Failures are #100476 and #99810

@jakobbotsch jakobbotsch merged commit a971763 into dotnet:main Apr 5, 2024
172 of 177 checks passed
@jakobbotsch jakobbotsch deleted the swift-reverse-pinvokes-structs branch April 5, 2024 08:04
matouskozak pushed a commit to matouskozak/runtime that referenced this pull request Apr 30, 2024
…#100344)

This adds the final support for frozen structs in UCO methods.

This passes 2500 auto generated tests locally on both macOS x64 and arm64. This
PR includes only 100 tests.

Frozen struct parameters are handled via the new ABI representation added in
dotnet#100138. When such a parameter exists we always allocate space for it on the
local stack frame. The struct is then reassembled from its passed constituents
as the first thing in the codegen.

One complication is that there can be an arbitrary amount of codegen to handle
this reassembling. We cannot handle an arbitrary amount of codegen in the
prolog, so the reassembling is handled in two places. First, since the amount of
register passed data is limited, we handle those in the prolog (which frees them
up to be used for other things). If some pieces were passed on the stack the JIT
then ensures that there is a scratch BB and generates the code to reassemble the
remaining parts as the first thing in the scratch BB.

Since Swift structs can be passed by reference in certain cases this PR also
enables `FEATURE_IMPLICIT_BYREFS` for SysV x64 to handle those cases. Depending
on the TP impact we can refine some of the ifdefs around this.
@github-actions github-actions bot locked and limited conversation to collaborators May 5, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants