JIT: Support frozen structs for Swift pinvokes #99294
Conversation
dotnet-issue-labeler
bot
added
the
area-CodeGen-coreclr
ghost
assigned 
|
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch Issue DetailsAdd support for decomposing structs in Swift pinvokes. Reverse pinvokes are not yet handled, and multireg returns also aren't yet handled. Only works with crossgen2/ILC since that's the only places Also add a bunch of auto generated Swift ABI stress tests. Currently 5000 tests, but I plan to reduce it to a much smaller set once everything is passing (it currently isn't).
|
This comment was marked as outdated.
This comment was marked as outdated.
…sure we have correct inlinearray support and fix unaligned lowering calculations
|
I put a PR out with the fix. |
|
Closing this temporarily to avoid running CI needlessly... |
This comment was marked as outdated.
This comment was marked as outdated.
|
@jkoritzinsky I auto generated 5000 tests in the |
|
I'll pull those into the unit testing harness and fix the failing cases. Thanks for generating them! |
| { | ||
| SwiftLoweringInterval& lastInterval = mergedIntervals[mergedIntervals.Size() - 1]; | ||
| lastInterval.size = interval.offset + interval.size - lastInterval.offset; | ||
| continue; | ||
| } |
There was a problem hiding this comment.
This logic didn't add any interval to mergedIntervals when this check failed.
| } | ||
|
|
||
| // Now we have the intervals, we can calculate the lowering. | ||
| CorInfoType loweredTypes[MAX_SWIFT_LOWERED_ELEMENTS]; | ||
| uint32_t offsets[MAX_SWIFT_LOWERED_ELEMENTS]; | ||
| uint32_t numLoweredTypes = 0; | ||
|
|
||
| for (uint32_t i = 0; i < mergedIntervals.Size(); i++, numLoweredTypes++) | ||
| for (uint32_t i = 0; i < mergedIntervals.Size(); i++) |
There was a problem hiding this comment.
Incrementing numLoweredTypes here would result in one extra increment for the opaque intervals.
|
cc @dotnet/jit-contrib PTAL @amanasifkhalid (JIT parts) @jkoritzinsky (VM changes) I tested with 5000 auto generated tests and they all pass with this PR with both crossgen2 and corerun. The PR here includes only 100 of the stress tests to avoid checking in hundreds of thousands of lines of test code. I added an The approach taken here is that we expand the frozen structs into the primitive arguments during import. In that way we guarantee no structs are left in the signature, and also that we are able to handle everything efficiently. Frozen struct returns and reverse pinvokes also remain to be supported. I'm going to work on the returns next, but I want to do that in a separate PR. |
src/coreclr/jit/importercalls.cpp
Outdated
| arg.SetWellKnownArg(WellKnownArg::SwiftSelf); | ||
| CORINFO_SWIFT_LOWERING* lowering = lowerings[argIndex]; | ||
|
|
||
| if (lowering == nullptr) |
There was a problem hiding this comment.
(lowering == nullptr) indicates that either the arg should be passed by reference, or that we never called getSwiftLowering for this arg (because it's a SwiftSelf, or it's not a value type), as the lowerings array is zero-initialized, right? It would sacrifice some perf, but this might read better if we always save the result of getSwiftLowering and check lowering->byReference here instead; that way, we know getSwiftLowering was never called for this arg if lowerings[argIndex] == nullptr, and can assert on it.
I don't have a strong preference on this, though.
There was a problem hiding this comment.
The SwiftSelf case is handled before we get here, but other than that you're right, lowering == nullptr here means that the arg is passed by reference. I'll switch it to always save the lowerings for all the struct args and add an assert -- I agree that seems better.
| using System.Runtime.InteropServices.Swift; | ||
| using Xunit; | ||
|
|
||
| public class SwiftAbiStress |
There was a problem hiding this comment.
Let's add this test to the Interop.csproj project directly and use the following attributes to do the correct test filtering (instead of issues.targets and the csproj filtering).
You'll also need to add a CMakeProjectReference to the CMakeLists..txt in this folder from Interop.csproj.
| public class SwiftAbiStress | |
| [PlatformSpecific(TestPlatforms.OSX | TestPlatforms.iOS | TestPlatforms.tvOS)] | |
| [ActiveIssue("https://github.com/dotnet/runtime/issues/94081", TestRuntimes.Mono)] | |
| public class SwiftAbiStress |
There was a problem hiding this comment.
Does it look good now?
I assume we'll want to put the other Swift tests under the same plan too (although let's do it separately from this PR).
There was a problem hiding this comment.
The change looks great, but it looks like there's some issues with a folder that only has targets on some platforms. I don't want to make you do the infra work to make this work correctly, so you can revert these changes.
We'll do it (for these tests and the other Swift tests) in a follow-up PR.
This reverts commit 4fcbe2a.
Add support for decomposing structs in Swift pinvokes. Reverse pinvokes are not yet handled, and multireg returns also aren't yet handled.
Also add 100 auto generated Swift ABI stress tests.