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

Fix AllocateRegularStaticBox for FOH #81857

Merged
merged 5 commits into from
Feb 9, 2023
Merged

Conversation

EgorBo
Copy link
Member

@EgorBo EgorBo commented Feb 8, 2023

No idea how that happened but apparently I forgot to revert this part in #79188 in one of the intermediate commits when I was blaming this part for a flaky NRE that turned out to be a missing GCPROTECT_BEGININTERIOR.

So this fixes boxed statics to have the expected codegen:

static DateTime MyField { get; set; }
; Assembly listing for method get_MyField():System.DateTime
-      48B8C81E802CFF010000 mov      rax, 0x1FF2C801EC8
-      488B00               mov      rax, gword ptr [rax]
-      488B4008             mov      rax, qword ptr [rax+08H]
+      48B8789AE568A4010000 mov      rax, 0x1A468E59A78
+      488B00               mov      rax, qword ptr [rax]
       C3                   ret 
-; Total bytes of code 18
+; Total bytes of code 14

Will try stress jobs once innerloop completes

Copy link
Member

@jkotas jkotas left a comment

Choose a reason for hiding this comment

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

Looks like the test failures are related.

@ghost ghost added the needs-author-action An issue or pull request that requires more info or actions from the author. label Feb 9, 2023
@EgorBo
Copy link
Member Author

EgorBo commented Feb 9, 2023

Right, it seems to surface an unrelated bug introduced recently. Most likely in #81651

Minimal repro:

public class Program
{
    static void Main(string[] args)
    {
        for (int i = 0; i < 100; i++)
        {
            GetNaturalColor(default);
            Thread.Sleep(16);
        }
    }

    internal static readonly VectorPacket256 DefaultColor = new (Vector256.Create(42.0f));

    internal struct VectorPacket256
    {
        public Vector256<float> Xs;
        public Vector256<float> Ys;
        public Vector256<float> Zs;

        public VectorPacket256(Vector256<float> init)
        {
            Xs = init;
            Ys = init;
            Zs = init;
        }
    }

    static int Foo() => 43;
    
    [MethodImpl(MethodImplOptions.NoInlining)]
    private static VectorPacket256 GetNaturalColor(Vector256<int> v)
    {
        var colors = DefaultColor;
        if (Foo() == 42)
            Console.WriteLine();
        return colors;
    }
}

@ghost ghost removed the needs-author-action An issue or pull request that requires more info or actions from the author. label Feb 9, 2023
@EgorBo
Copy link
Member Author

EgorBo commented Feb 9, 2023

/azp run runtime-coreclr outerloop, runtime-coreclr gcstress0x3-gcstress0xc

@azure-pipelines
Copy link

Azure Pipelines successfully started running 2 pipeline(s).

Copy link
Member

@tannergooding tannergooding left a comment

Choose a reason for hiding this comment

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

JIT side change LGTM

Co-authored-by: SingleAccretion <62474226+SingleAccretion@users.noreply.github.com>
@EgorBo EgorBo merged commit 7f73d0c into dotnet:main Feb 9, 2023
@EgorBo EgorBo deleted the fix-foh-static-box branch February 9, 2023 18:34
@ghost ghost locked as resolved and limited conversation to collaborators Mar 11, 2023
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