Skip to content
This repository was archived by the owner on Nov 1, 2020. It is now read-only.

Conversation

@MichalStrehovsky
Copy link
Member

An alternate approach to what was attempted in #3962 (and dotnet/coreclr#12420), this time leveraging preexisting infrastructure in RyuJIT built for accessing RVA static fields. This is limited to non-GC statics, since that's what RyuJIT has.

It feels a bit more natural to how JitInterface is structured (e.g. RyuJIT is calling initClass to eliminate the need to do the cctor check in some cases). If this is something we would want to take further, the next steps would be:

  1. Update initClass to allow generating the inline cctor check (we currently call the ReadyToRun helper to get non-GC static base - this change doesn't affect that).
  2. Change CORINFO_FIELD_INFO - replace CORINFO_CONST_LOOKUP fieldLookup with CORINFO_LOOKUP fieldLookup so that we can also do this from shared code.
  3. Some updates to handle GC statics and thread statics that I haven't thought about too much yet.

I don't have plans to do the next steps anytime soon though.

The changes to the generated code look like this:

internal class Program
{
    static int X;
    static int Y;

    private static int Main()
    {
        return X + Y;
    }
}

Before:

call        __GetNonGCStaticBase_repro_Program (07FF65ADE2FCCh)  
mov         edx,dword ptr [rax]  
add         edx,dword ptr [rax+4]  
mov         eax,edx  

After:

mov         eax,dword ptr [repro_Program::__NONGCSTATICS (07FF66540BBD8h)]  
add         eax,dword ptr [repro_Program::__NONGCSTATICS+4h (07FF66540BBDCh)]  

@MichalStrehovsky
Copy link
Member Author

This is still asserting RyuJIT. This looks like another RyuJIT bug in the rarely used area but this time I don't have a repro with CoreCLR.

I'm going to let this pull request sit until we decide that this is the direction we want. I don't want to engage RyuJIT team on this unless we have to.

@jkotas
Copy link
Member

jkotas commented Dec 19, 2017

RyuJIT is calling initClass to eliminate the need to do the cctor check in some cases
Change CORINFO_FIELD_INFO - replace CORINFO_CONST_LOOKUP fieldLookup with CORINFO_LOOKUP fieldLookup

These parts sound good to me.

SymbolWithOffset

I think that it would be better to keep providing baseAddress + offset and let the JIT to figure out the address mode. It is beneficial on non-Intel where computing absolute address is relatively expensive and so you want JIT to CSE the baseAddress. (Intel can benefit from it in some less common cases - when optimizing for code size.)

@jkotas
Copy link
Member

jkotas commented Apr 19, 2020

Closing stale PRs

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.

2 participants