Skip to content

Conversation

@jkoritzinsky
Copy link
Member

Implement Swift's physical lowering pass by re-using the interval traversal algorithm used in the explicit layout validator.

This does not hook it up to the JIT interface yet.

For the JIT interface API, I was thinking something like: BOOL getSwiftLowering(CORINFO_CLASS_HANDLE clsHnd, uint8_t numLoweredElements, CORINFO_TYPE loweredElements[4]);

This function would return FALSE for any types that should be passed by ref, and the loweredElements array would represent the equivalent primitive sequence (ending with CORINFO_TYPE_EMPTY if there are less than 4 primitives).

Implement Swift's physical lowering pass by re-using the interval traversal algorithm used in the explicit layout validator.

This does not hook it up to the JIT interface yet.
@ghost
Copy link

ghost commented Feb 23, 2024

Tagging subscribers to this area: @agocke, @MichalStrehovsky, @jkotas
See info in area-owners.md if you want to be subscribed.

Issue Details

Implement Swift's physical lowering pass by re-using the interval traversal algorithm used in the explicit layout validator.

This does not hook it up to the JIT interface yet.

For the JIT interface API, I was thinking something like: BOOL getSwiftLowering(CORINFO_CLASS_HANDLE clsHnd, uint8_t numLoweredElements, CORINFO_TYPE loweredElements[4]);

This function would return FALSE for any types that should be passed by ref, and the loweredElements array would represent the equivalent primitive sequence (ending with CORINFO_TYPE_EMPTY if there are less than 4 primitives).

Author: jkoritzinsky
Assignees: jkoritzinsky
Labels:

area-NativeAOT-coreclr

Milestone: -

@jakobbotsch
Copy link
Member

jakobbotsch commented Feb 23, 2024

For the JIT interface API, I was thinking something like: BOOL getSwiftLowering(CORINFO_CLASS_HANDLE clsHnd, uint8_t numLoweredElements, CORINFO_TYPE loweredElements[4]);

bool as a return value sounds like the API can fail. I would model it after getSystemVAmd64PassStructInRegisterDescriptor, e.g. something like

struct CORINFO_SWIFT_LOWERING
{
  bool byReference;
  CORINFO_ELEMENT_TYPE loweredElements[4];
  size_t numLoweredElements;
};

void getSwiftTypeLowering(CORINFO_CLASS_HANDLE clsHnd, CORINFO_SWIFT_LOWERING* result)

@kotlarmilos kotlarmilos added this to the 9.0.0 milestone Feb 23, 2024
@jakobbotsch
Copy link
Member

jakobbotsch commented Feb 28, 2024

I hit an assert in the lowering for this type:

struct F5_S1_S0
{
    public short F0;
}

struct F5_S1
{
    public ulong F0;
    public long F1;
    public F5_S1_S0 F2;
    public long F3;
}

(https://github.com/dotnet/runtime/compare/main...jakobbotsch:runtime:swift-abi?expand=1 is a branch based on this PR that hooks up the JIT side and has a bunch of tests. Not sure that the JIT side works yet.)

Copy link
Member

@jakobbotsch jakobbotsch left a comment

Choose a reason for hiding this comment

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

LGTM, but you may want to get approval from someone more well-versed in the managed type system.

The support here is also for crossgen2, right? It would be good to update the PR title to reflect that if so.

@jkoritzinsky jkoritzinsky changed the title Implement Swift's physical lowering pass for NativeAOT Implement Swift's physical lowering pass in the managed type system Mar 1, 2024
Copy link
Member

@MichalStrehovsky MichalStrehovsky left a comment

Choose a reason for hiding this comment

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

Only looked at how the type system is used. Not reviewing the lowering logic or the correctness of ExplicitLayoutValidator refactor.


Debug.Assert(PointerSize == 8, "Swift interop is only supported on 64-bit platforms.");

if (fieldType.Category is TypeFlags.IntPtr or TypeFlags.UIntPtr || fieldType.IsPointer || fieldType.IsFunctionPointer)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if (fieldType.Category is TypeFlags.IntPtr or TypeFlags.UIntPtr || fieldType.IsPointer || fieldType.IsFunctionPointer)
if (fieldType.Category is TypeFlags.IntPtr or TypeFlags.UIntPtr or TypeFlags.Pointer or TypeFlags.FunctionPointer)

Copy link
Member

@kotlarmilos kotlarmilos left a comment

Choose a reason for hiding this comment

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

LGTM! I've left one comment for consideration. How will the JIT use it? Is it possible to share this implementation with the Mono runtime so that there is a unified implementation?

List<CorInfoType> loweredTypes = visitor.GetLoweredTypeSequence();

// If a type has a primitive sequence with more than 4 elements, Swift passes it by reference.
if (loweredTypes.Count > 4)
Copy link
Member

Choose a reason for hiding this comment

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

Does it make sense to implement an early check in the GetLoweredTypeSequence and avoid lowering the full sequence if loweredTypes.Count > 4?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think it's fine. The number of frozen structs that would surpass the max size and be passed by-reference is likely very small (as if the type is being passed by-ref, it's not getting most of the benefits that being frozen allows).

@jakobbotsch
Copy link
Member

The algorithm lowers

struct F2087_S0_S0
{
	public double F0;
	public float F1;
}

struct F2087_S0
{
	public F2087_S0_S0 F0;
	public int F1;
	public sbyte F2;
}

to

    [0] double
    [1] float
    [2] long
    [3] byte

F2087_S0 is a 24 byte sized struct, but the returned lowering implies that [3] byte starts at offset 24 in the struct. Is it a bug in the lowering?

@jakobbotsch
Copy link
Member

jakobbotsch commented Mar 4, 2024

struct F114_S0
{
	public float F0;
	public ushort F1;
	public short F2;
	public ushort F3;
}

seems to be lowered to

  Argument 0 of type SwiftAbiStress+F114_S0 must be passed as 2 primitive(s)
    [0] float
    [1] long

however, Swift seems to lower it to float, i32, i16: https://godbolt.org/z/WrE8ETzcb

(Feel free to merge the PR and fix these separately. I can open my PR once you do that, which may give you a better way to get access to test cases.)

@jkoritzinsky
Copy link
Member Author

How will the JIT use it? Is it possible to share this implementation with the Mono runtime so that there is a unified implementation?

The JIT will use this through the EE-JIT interface. We are going to need a separate implementation for CoreCLR (we can't use the managed type system there), so we can try to keep it to two implementations instead of three, but I'm not sure how easy that will be.

@jkoritzinsky
Copy link
Member Author

@jakobbotsch I found the issue. My implementation of the algorithm wasn't taking into account that we need to match padding of the original struct type. I've adjusted the algorithm and am pushing out a fix.

@jkoritzinsky
Copy link
Member Author

Timeout is unrelated, merging in.

@jkoritzinsky jkoritzinsky merged commit 68f7885 into dotnet:main Mar 4, 2024
@jkoritzinsky jkoritzinsky deleted the swift-lowering branch March 4, 2024 22:53
@github-actions github-actions bot locked and limited conversation to collaborators Apr 4, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants