Skip to content

JIT: Support loading Swift self register from SwiftSelf struct#99132

Merged
amanasifkhalid merged 4 commits intodotnet:mainfrom
amanasifkhalid:swift-self-reg
Mar 1, 2024
Merged

JIT: Support loading Swift self register from SwiftSelf struct#99132
amanasifkhalid merged 4 commits intodotnet:mainfrom
amanasifkhalid:swift-self-reg

Conversation

@amanasifkhalid
Copy link
Contributor

@amanasifkhalid amanasifkhalid commented Feb 29, 2024

Adds JIT support for loading the Swift self/context register value from the SwiftSelf struct argument passed to a Swift call. Also enables corresponding tests.

@ghost ghost assigned amanasifkhalid Feb 29, 2024
@ghost ghost added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Feb 29, 2024
@ghost
Copy link

ghost commented Feb 29, 2024

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

Issue Details

Adds JIT support for loading the Swift self/context register value from the SwiftSelf struct argument passed to a Swift call. Also enables corresponding tests.

Author: amanasifkhalid
Assignees: amanasifkhalid
Labels:

area-CodeGen-coreclr
Milestone: -

@amanasifkhalid amanasifkhalid marked this pull request as ready for review March 1, 2024 04:19
@amanasifkhalid
Copy link
Contributor Author

amanasifkhalid commented Mar 1, 2024

cc @jakobbotsch @kotlarmilos @jkoritzinsky. Here's a snippet of what the codegen looks like on x64, where the self register is r13:

       mov      r13, qword ptr [rbp-0x48]
       mov      rax, 0x7FF8FF2F59B8      ; function address
       mov      qword ptr [rbp-0x90], rax
       lea      rax, G_M44653_IG07
       mov      qword ptr [rbp-0x78], rax
       mov      rax, qword ptr [rbp-0x58]
       lea      rcx, bword ptr [rbp-0xA0]
       mov      qword ptr [rax+0x10], rcx
       mov      rax, qword ptr [rbp-0x58]
       mov      byte  ptr [rax+0x0C], 0
       call     [SelfContextTests:getMagicNumber(System.Runtime.InteropServices.Swift.SwiftSelf):long]

And on arm64, where the self register is x20:

            ldr     x20, [fp, #0x80]	// [V01 loc1]
            movz    x0, #0x4988
            movk    x0, #0x2103 LSL #16
            movk    x0, #0x7FF9 LSL #32
            str     x0, [fp, #0x30]	// [V05 PInvokeFrame+0x18]
            adr     x0, [G_M44653_IG09]
            str     x0, [fp, #0x48]	// [V05 PInvokeFrame+0x30]
            ldr     x0, [fp, #0x70]	// [V04 FramesRoot]
            add     x1, fp, #32	// [V05 PInvokeFrame+0x08]
            str     x1, [x0, #0x10]
            ldr     x0, [fp, #0x70]	// [V04 FramesRoot]
            strb    wzr, [x0, #0x0C]
            movz    x0, #0x49C8      // code for SelfContextTests:getMagicNumber(System.Runtime.InteropServices.Swift.SwiftSelf):long
            movk    x0, #0x2103 LSL #16
            movk    x0, #0x7FF9 LSL #32
            ldr     x0, [x0]
            blr     x0

jakobbotsch
jakobbotsch reviewed Mar 1, 2024
View reviewed changes
src/coreclr/jit/morph.cpp
Comment on lines +3184 to 3187
// Exception: Lower SwiftSelf struct arg to GT_LCL_FLD
if (call->gtArgs.IsNonStandard(this, call, &arg) && arg.AbiInfo.IsPassedInRegisters() &&
(arg.GetWellKnownArg() != WellKnownArg::SwiftSelf))
{
Copy link
Member

@jakobbotsch jakobbotsch Mar 1, 2024

Choose a reason for hiding this comment

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

Why is this necessary? That doesn't seem right.

Copy link
Member

@jakobbotsch jakobbotsch Mar 1, 2024

Choose a reason for hiding this comment

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

Likely we should instead insert a new arg in the importer that represents "self" as a primitive instead of keeping this as a struct argument. It being a struct argument is just an artifact of the design and it wouldn't work to keep it as a struct on all ABIs.

Copy link
Member

@jakobbotsch jakobbotsch Mar 1, 2024

Choose a reason for hiding this comment

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

Let's just keep it as is for now. I'm refactoring stuff for the struct work which should make this change simple to make.

Copy link
Contributor Author

@amanasifkhalid amanasifkhalid Mar 1, 2024

Choose a reason for hiding this comment

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

Likely we should instead insert a new arg in the importer that represents "self" as a primitive instead of keeping this as a struct argument.

Yeah, the fact that the argument was being kept as TYP_STRUCT was causing asserts in later phases.

Let's just keep it as is for now. I'm refactoring stuff for the struct work which should make this change simple to make.

Sure thing.

jakobbotsch
jakobbotsch reviewed Mar 1, 2024
View reviewed changes
jakobbotsch
jakobbotsch reviewed Mar 1, 2024
View reviewed changes
src/coreclr/jit/gentree.h
class CallArgs;

enum class WellKnownArg
enum class WellKnownArg : unsigned
Copy link
Member

@jakobbotsch jakobbotsch Mar 1, 2024

Choose a reason for hiding this comment

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

Why this change? (Seems fine, just curious)

Copy link
Contributor Author

@amanasifkhalid amanasifkhalid Mar 1, 2024

Choose a reason for hiding this comment

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

CallArg::m_wellKnownArg is 5 bits, and WellKnownArg::SwiftSelf has a value of 16 (also 5 bits). If m_wellKnownArg is signed, then we lose the most-significant bit, so comparisons like m_wellKnownArg == WellKnownArg::SwiftSelf fail.

@amanasifkhalid
Copy link
Contributor Author

amanasifkhalid commented Mar 1, 2024

ryujit-bot doesn't seem to be running, so I'll point out here that this is a no-diff change.

@amanasifkhalid amanasifkhalid merged commit f135699 into dotnet:main Mar 1, 2024
@amanasifkhalid amanasifkhalid mentioned this pull request Mar 1, 2024
Closed
42 tasks
@amanasifkhalid amanasifkhalid deleted the swift-self-reg branch March 1, 2024 15:53
@github-actions github-actions bot locked and limited conversation to collaborators Apr 1, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@jakobbotsch jakobbotsch jakobbotsch approved these changes

Assignees

@amanasifkhalid amanasifkhalid

Labels

area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@amanasifkhalid @jakobbotsch