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

[mono] Add SwiftError support for Swift reverse pinvokes #101122

Merged
merged 7 commits into from
May 6, 2024

Conversation

jkurdek
Copy link
Member

@jkurdek jkurdek commented Apr 16, 2024

Adds support for SwiftError in Swift reverse pinvokes. With this changes basic pinvokes using primitive types and SwiftSelf/SwiftError arguments should work. Contributes to #100010

Works on arm64/amd64.

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.

Thanks, looks great! Do you plan to include the amd64 support in this PR?

code = emit_strx (code, ARMREG_R21, ARMREG_IP0, 0);
} else if (cfg->method->wrapper_type == MONO_WRAPPER_NATIVE_TO_MANAGED) {
code = emit_ldrx (code, ARMREG_R21, cfg->arch.swift_error_var->inst_basereg, GTMREG_TO_INT (cfg->arch.swift_error_var->inst_offset));
code = emit_ldrx (code, ARMREG_R21, ARMREG_R21, 0);
Copy link
Member

Choose a reason for hiding this comment

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

Please add a comment here for clarity.

}
if (cfg->method->wrapper_type == MONO_WRAPPER_NATIVE_TO_MANAGED) {
code = emit_ldrx (code, ARMREG_IP0, cfg->arch.swift_error_var->inst_basereg, GTMREG_TO_INT (cfg->arch.swift_error_var->inst_offset));
code = emit_strx (code, ARMREG_R21, ARMREG_IP0, 0);
Copy link
Member

Choose a reason for hiding this comment

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

Does this align with the CoreCLR implementation? It seems intuitive to load swifterror reg into the SwiftError argument, but I wonder if such cases are expected.

Copy link
Member

@amanasifkhalid amanasifkhalid Apr 16, 2024

Choose a reason for hiding this comment

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

For the CoreCLR implementation, RyuJIT does not load the error register's current value into the SwiftError arg upon method entry. The JIT implementation represents the error value by creating a SwiftError "pseudo-local", and converts all usages of the SwiftError* out parameter into usages of the local's address (see #100692). This approach allows us to be quite liberal in optimizing uses of the SwiftError* parameter (for example, we can promote the SwiftError::Value member to its own local, and enregister it), but with our implementation, I believe the initial error value upon method entry is undefined.

Copy link
Member Author

Choose a reason for hiding this comment

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

@kotlarmilos do we want to remove loading swifterror reg into the SwiftError to align?

Copy link
Member

Choose a reason for hiding this comment

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

I believe it's beneficial to align with the CoreCLR implementation. However, since it is undefined and if there are no performance implications, we can leave it as is.

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 so far -- thanks!

@@ -2887,7 +2887,8 @@ mono_arch_allocate_vars (MonoCompile *cfg)
offset += size;

cfg->arch.swift_error_var = ins;
cfg->used_int_regs |= 1 << ARMREG_R21;
if (cfg->method->wrapper_type == MONO_WRAPPER_MANAGED_TO_NATIVE)
cfg->used_int_regs |= 1 << ARMREG_R21;
Copy link
Member

Choose a reason for hiding this comment

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

It might be helpful to explain here that in the NATIVE_TO_MANAGED case, the error register is functioning as an extra return register, and thus isn't treated as callee-save.

@jkurdek jkurdek changed the title [Mono] Add basic support for Swift reverse pinvokes [mono] Add SwiftError support for Swift reverse pinvokes Apr 17, 2024
@matouskozak
Copy link
Member

matouskozak commented Apr 18, 2024

Good job! Note for the CI test coverage, we have a good coverage for osx-x64 (mini and interpreter) on the runtime pipeline. For arm64, we only have interpreter coverage running on runtime-extra-platforms pipeline.

@kotlarmilos
Copy link
Member

/azp run runtime-extra-platforms

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

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!

offset = ALIGN_TO (offset, sizeof (target_mgreg_t));
ins->inst_basereg = cfg->frame_reg;
ins->inst_offset = offset;
Copy link
Member

@kotlarmilos kotlarmilos Apr 25, 2024

Choose a reason for hiding this comment

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

Why it doesn't include the ARGS_OFFSET offset between fp and the first argument in the callee?

Copy link
Member Author

Choose a reason for hiding this comment

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

ainfo->offset was always 0 here. So it was always allocated at ARGS_OFFSET from the beginning. I think that offset already includes information about the distance.

Copy link
Member

Choose a reason for hiding this comment

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

The Swift types are specified either at the beginning or the end of signature. We don't enforce strict signature rules, but it would be beneficial to make them more general if possible. Can it still function if SwiftError is specified as the last argument, using ainfo->offset instead of offset?

@amanasifkhalid, what are the limitations from the CoreCLR side?

Copy link
Member

Choose a reason for hiding this comment

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

We don't enforce any signature rules, either. The Swift special register types can appear anywhere in the signature's parameter list.

Copy link
Member Author

@jkurdek jkurdek Apr 25, 2024

Choose a reason for hiding this comment

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

@kotlarmilos i think for that to work with ainfo->offset without any modifications, the error would have to be the first argument. We could probably update ainfo->offset in get_call_info so that it works for every position. I'm not sure what are the benefits of using ainfo->offset though, as the current solution works for every position.

Copy link
Member

@kotlarmilos kotlarmilos Apr 26, 2024

Choose a reason for hiding this comment

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

Ok, sounds good. I forgot that the ainfo->offset is always 0 for swifterror in the get_call_info.

@jkurdek
Copy link
Member Author

jkurdek commented Apr 26, 2024

@lambdageek could you take a look?

Copy link
Member

@lambdageek lambdageek left a comment

Choose a reason for hiding this comment

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

looks good to me. although by this point you and Milos know way more about how the calling convention code works.

@jkurdek jkurdek merged commit 5e949e1 into dotnet:main May 6, 2024
122 of 127 checks passed
michaelgsharp pushed a commit to michaelgsharp/runtime that referenced this pull request May 9, 2024
* initialized swift reverse pinvokes for mini jit arm64

* added comments

* fixed reverse pinvokes error passing on arm64

* implemented swift reverse pinvoke error passing on amd64

* disable SwiftErrorHandling tests on mono interpreter for now

* add comments
Ruihan-Yin pushed a commit to Ruihan-Yin/runtime that referenced this pull request May 30, 2024
* initialized swift reverse pinvokes for mini jit arm64

* added comments

* fixed reverse pinvokes error passing on arm64

* implemented swift reverse pinvoke error passing on amd64

* disable SwiftErrorHandling tests on mono interpreter for now

* add comments
@github-actions github-actions bot locked and limited conversation to collaborators Jun 6, 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.

5 participants