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

Test case failure with TieredPGO #52975

Closed
AndyAyersMS opened this issue May 19, 2021 · 1 comment · Fixed by #52998
Closed

Test case failure with TieredPGO #52975

AndyAyersMS opened this issue May 19, 2021 · 1 comment · Fixed by #52998
Assignees
Labels
area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI
Milestone

Comments

@AndyAyersMS
Copy link
Member

@DrewScoggins ran into failures trying to enable TieredPGO for microbenchmarks. This bug is distilled from the failure that happens there (Json.NET is enumerating a dictionary for serialization when producing the lab report data):

using System;
using System.Collections;
using System.Collections.Generic;
using System.Threading;

class X
{
    static int F(IDictionary i)
    {
        int r = 0;
        IDictionaryEnumerator e = i.GetEnumerator();
        while (e.MoveNext())
        {
            DictionaryEntry entry = e.Entry;
            r++;
        }
        return r;
    }

    public static int Main()
    {
        Dictionary<string, string> s = new Dictionary<string, string>();
        s["hello"] = "world";
        int r = 0;

        for (int i = 0; i < 50; i++)
        {
            r += F(s);
            Thread.Sleep(15);
        }

        int iter = 100;

        for (int i = 0; i < iter; i++)
        {
            r += F(s);
        }

        int result = 2 * (r - iter);
        Console.WriteLine($"Result={result}");
        return result;
    }
}

with

complus_tc_quickjitforloops=1
complus_tieredpgo=1

and it will fail when newing up an object in GetEnumerator:

Assert failure(PID 38060 [0x000094ac], Thread: 20964 [0x51e4]): AsMethodTable()->SanityCheck()

CORECLR! TypeHandle::Verify + 0x6B (0x00007fff`d1ffeaf3)
CORECLR! JIT_New + 0x198 (0x00007fff`d214eaa8)
<no module>! <no symbol> + 0x0 (0x00007fff`729fc690)
<no module>! <no symbol> + 0x0 (0x00007fff`72d3c740)
<no module>! <no symbol> + 0x0 (0x00000262`35fe3ff0)
    File: C:\repos\runtime1\src\coreclr\vm\typehandle.cpp Line: 51
    Image: c:\repos\runtime1\artifacts\tests\coreclr\Windows.x64.checked\Tests\Core_Root\corerun.exe

000000FBCDF7E168 00007fffcb2dcf04 [HelperMethodFrame: 000000fbcdf7e168] 
000000FBCDF7E280 00007fff6bcac690 System.Collections.Generic.Dictionary`2[[System.__Canon, System.Private.CoreLib],[System.__Canon, System.Private.CoreLib]].System.Collections.IDictionary.GetEnumerator()
000000FBCDF7E300 00007fff6bcac569 X.F(System.Collections.IDictionary)
000000FBCDF7E350 00007fff6bca9450 X.Main()

This still fails if the only method jitted with tiered PGO is X.F. This can be done in a checked jit by specifying its hash, like so:

set complus_jitenablepgorange=5162f578
@dotnet-issue-labeler dotnet-issue-labeler bot added area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI untriaged New issue has not been triaged by the area owner labels May 19, 2021
@AndyAyersMS AndyAyersMS self-assigned this May 19, 2021
@AndyAyersMS AndyAyersMS removed the untriaged New issue has not been triaged by the area owner label May 19, 2021
@AndyAyersMS AndyAyersMS added this to the 6.0.0 milestone May 19, 2021
@AndyAyersMS AndyAyersMS added this to Needs Triage in .NET Core CodeGen via automation May 19, 2021
@AndyAyersMS AndyAyersMS moved this from Needs Triage to PGO in .NET Core CodeGen May 19, 2021
@AndyAyersMS
Copy link
Member Author

Seems like the bug is when we devirtualize the call to the enumerator's get_Entry -- this call requires an inst table param, but it also returns a struct via a hidden return buffer, and so we get the updated arg list wrong, and the call writes into the method table, and that's not good.

That is, instead of (byref (this), byref(retval), typeCtxt, ...) we pass (byref(this), typeCtxt, byref(retval).

AndyAyersMS added a commit to AndyAyersMS/runtime that referenced this issue May 19, 2021
If a value class method returns a struct, and its unboxed entry point
requires a type context argument, make sure to pass the context argument
properly.

Also, fetch the type context (method table) from the box, rather than
creating it from the class handle we have on hand; this tends to produce
smaller code as we're often fetching the method table for other reasons
anyways.

Closes dotnet#52975.
@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label May 19, 2021
.NET Core CodeGen automation moved this from PGO to Done May 20, 2021
AndyAyersMS added a commit that referenced this issue May 20, 2021
If a value class method returns a struct, and its unboxed entry point
requires a type context argument, make sure to pass the context argument
properly.

Also, fetch the type context (method table) from the box, rather than
creating it from the class handle we have on hand; this tends to produce
smaller code as we're often fetching the method table for other reasons
anyways.

Closes #52975.
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label May 20, 2021
@ghost ghost locked as resolved and limited conversation to collaborators Jun 19, 2021
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
Archived in project
Development

Successfully merging a pull request may close this issue.

1 participant