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

Remove LocAllocSP for non-x86 platforms #11269

Closed
BruceForstall opened this issue Oct 19, 2018 · 3 comments
Closed

Remove LocAllocSP for non-x86 platforms #11269

BruceForstall opened this issue Oct 19, 2018 · 3 comments
Assignees
Labels
area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI
Milestone

Comments

@BruceForstall
Copy link
Member

In functions with localloc, we allocate a stack variable to store the RSP after the most recent localloc. This is named the LocAllocSP variable in the JIT. But it doesn’t look like it’s ever read; it’s only written. And it isn’t reported in the GC info.

So why do we do it? Is it a holdover from some older JIT32 implementation?

The one subtlety to investigate is whether the VM uses this in some way. Note that in some cases, especially for x86, we don't explicitly report all variable stack locations -- we report one, and then expect others to exist and be in a particular relative order.

@BruceForstall BruceForstall self-assigned this Oct 19, 2018
@BruceForstall
Copy link
Member Author

genLclHeap comment says:

//      We store the ESP after the localloc is complete in the LocAllocSP
//      variable. This variable is implicitly reported to the VM in the GC info (its position
//      is defined by convention relative to other items), and is used by the GC to find the
//      "base" stack pointer in functions with localloc.

It's not clear this is needed/used outside of x86. But will close this issue anyway as "won't fix".

@BruceForstall
Copy link
Member Author

lvaMarkLocalVars comment says:

        // TODO: LocAllocSPvar should be only required by the implicit frame layout expected by the VM on x86.
        // It should be removed on other platforms once we check there are no other implicit dependencies.

@BruceForstall BruceForstall reopened this Oct 19, 2018
@BruceForstall
Copy link
Member Author

I've verified that this is only needed for the JIT32 GC encoder, thus, only for x86. I'll submit a PR to remove it for other platforms, with this comment:

        // LocAllocSPvar is only required by the implicit frame layout expected by the VM on x86. Whether
        // a function contains a Localloc is conveyed in the GC information, in the InfoHdrSmall.localloc
        // field. The function must have an EBP frame. Then, the VM finds the LocAllocSP slot by assuming
        // the following stack layout:
        //
        //      -- higher addresses --
        //      saved EBP                       <-- EBP points here
        //      other callee-saved registers    // InfoHdrSmall.savedRegsCountExclFP specifies this size
        //      optional GS cookie              // InfoHdrSmall.security is 1 if this exists
        //      LocAllocSP slot
        //      -- lower addresses --
        // 
        // See also eetwain.cpp::GetLocallocSPOffset() and its callers.

@BruceForstall BruceForstall changed the title Remove LocAllocSP Remove LocAllocSP for non-x86 platforms Oct 23, 2018
@msftgits msftgits transferred this issue from dotnet/coreclr Jan 31, 2020
@msftgits msftgits added this to the Future milestone Jan 31, 2020
@ghost ghost locked as resolved and limited conversation to collaborators Dec 15, 2020
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

No branches or pull requests

2 participants