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

JIT fails to track exact class handles with ternaries in initializers #55079

Open
jakobbotsch opened this issue Jul 2, 2021 · 7 comments
Open
Assignees
Labels
area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI
Milestone

Comments

@jakobbotsch
Copy link
Member

jakobbotsch commented Jul 2, 2021

public static int Main()
{
    O o = new O { Arg = Environment.TickCount < 0 ? 0 : 1 };
    return o.ToString().Length;
}
private class O
{
    public int Arg;
    public override string ToString() => "O";
}

We don't manage to devirtualize the ToString call here because we fail to track the exact class handle. The IL generated seems a bit unfortunate for us:

IL to import:
IL_0000  73 04 00 00 06    newobj       0x6000004
IL_0005  25                dup
IL_0006  28 01 00 00 0a    call         0xA000001
IL_000b  16                ldc.i4.0
IL_000c  32 03             blt.s        3 (IL_0011)
IL_000e  17                ldc.i4.1
IL_000f  2b 01             br.s         1 (IL_0012)
IL_0011  16                ldc.i4.0
IL_0012  7d 01 00 00 04    stfld        0x4000001
IL_0017  6f 02 00 00 0a    callvirt     0xA000002
IL_001c  6f 03 00 00 0a    callvirt     0xA000003
IL_0021  2a                ret

The code ends up as

; Assembly listing for method Program:Main():int
; Emitting BLENDED_CODE for X64 CPU with AVX - Windows
; optimized code
; rsp based frame
; partially interruptible
; No PGO data
; 0 inlinees with PGO data; 2 single block inlinees; 0 inlinees without PGO data
; Final local variable assignments
;
;  V00 OutArgs      [V00    ] (  1,  1   )  lclBlk (32) [rsp+00H]   "OutgoingArgSpace"
;  V01 tmp1         [V01,T00] (  2,  4   )     ref  ->  rax         class-hnd exact "NewObj constructor temp"
;  V02 tmp2         [V02,T05] (  2,  2   )     ref  ->  rsi
;  V03 tmp3         [V03,T02] (  3,  2   )     ref  ->  rsi
;  V04 tmp4         [V04,T01] (  5,  3   )     ref  ->  rcx
;  V05 tmp5         [V05,T03] (  3,  2   )     ref  ->  rax
;  V06 tmp6         [V06,T04] (  3,  2   )     int  ->  rdx
;
; Lcl frame size = 32

G_M24375_IG01:              ;; offset=0000H
       56                   push     rsi
       4883EC20             sub      rsp, 32
                                                ;; bbWeight=1    PerfScore 1.25
G_M24375_IG02:              ;; offset=0005H
       48B9E857E156FE7F0000 mov      rcx, 0x7FFE56E157E8
       E84C46A55F           call     CORINFO_HELP_NEWSFAST
       488BF0               mov      rsi, rax
       E8F483B95F           call     System.Environment:get_TickCount():int
       85C0                 test     eax, eax
       7C0D                 jl       SHORT G_M24375_IG04
                                                ;; bbWeight=1    PerfScore 3.75
G_M24375_IG03:              ;; offset=0020H
       488BCE               mov      rcx, rsi
       488BC1               mov      rax, rcx
       BA01000000           mov      edx, 1
       EB08                 jmp      SHORT G_M24375_IG05
                                                ;; bbWeight=0.50 PerfScore 1.38
G_M24375_IG04:              ;; offset=002DH
       488BCE               mov      rcx, rsi
       488BC1               mov      rax, rcx
       33D2                 xor      edx, edx
                                                ;; bbWeight=0.50 PerfScore 0.38
G_M24375_IG05:              ;; offset=0035H
       895008               mov      dword ptr [rax+8], edx
       488B0541CE3300       mov      rax, qword ptr [(reloc 0x7ffe56e15830)]
       FF5008               call     gword ptr [rax+8]System.Object:ToString():System.String:this
       8B4008               mov      eax, dword ptr [rax+8]
                                                ;; bbWeight=1    PerfScore 8.00
G_M24375_IG06:              ;; offset=0045H
       4883C420             add      rsp, 32
       5E                   pop      rsi
       C3                   ret
                                                ;; bbWeight=1    PerfScore 1.75

; Total bytes of code 75, prolog size 5, PerfScore 24.00, instruction count 22, allocated bytes for code 75 (MethodHash=d9b8a0c8) for method Program:Main():int

Funnily enough, value numbering and constant prop ends up loading the method table chunk as a constant.

cc @dotnet/jit-contrib

category:correctness
theme:zero-init
skill-level:expert
cost:medium
impact:medium

@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 Jul 2, 2021
@EgorBo
Copy link
Member

EgorBo commented Jul 2, 2021

A potential fix: EgorBo@692a597
but I'm not sure it's safe to set SingleDef flag here for such spills.

@jakobbotsch jakobbotsch added untriaged New issue has not been triaged by the area owner and removed untriaged New issue has not been triaged by the area owner labels Jul 2, 2021
@JulieLeeMSFT JulieLeeMSFT removed the untriaged New issue has not been triaged by the area owner label Jul 6, 2021
@JulieLeeMSFT JulieLeeMSFT added this to the 6.0.0 milestone Jul 6, 2021
@JulieLeeMSFT
Copy link
Member

Setting this to .NET 6 for now. If time is not enough, we can move to future.

@EgorBo
Copy link
Member

EgorBo commented Jul 12, 2021

Moving to 7.0.0, keeping type info for spilled to stack locals looks tricky.

@EgorBo EgorBo modified the milestones: 6.0.0, 7.0.0 Jul 12, 2021
@EgorBo
Copy link
Member

EgorBo commented Jul 14, 2021

using System.Runtime.CompilerServices;
using System.Threading;

public static class Tests
{
    static void Main()
    {
        BFactory factory = new BFactory();
        for (int i = 0; i < 100; i++)
        {
            // Test1 is GDV friendly and has "return 42"
            //Test1(factory);

            // Test2 is not
            Test2(factory);

            Thread.Sleep(16);
        }
    }

    [MethodImpl(MethodImplOptions.NoInlining)]
    static int Test1(AFactory factory)
    {
        if (factory != null)
        {
            A a = factory.GetA();
            if (a != null)
            {
                return a.GetValue();
            }
        }
        return 0;
    }

    [MethodImpl(MethodImplOptions.NoInlining)]
    static int Test2(AFactory factory)
    {
        return factory?.GetA()?.GetValue() ?? 0;
    }
}


public class A
{
    public virtual int GetValue() => 41;
}

public class B : A
{
    public override int GetValue() => 42;
}

public class AFactory
{
    public virtual A GetA() => null;
}

public class BFactory : AFactory
{
    private static B b = new B();

    public override A GetA() => b;
}

A similar issue
@AndyAyersMS in the code above Test1() and Test2() are semantically the same, but in Test2() GetValue is not devirtualized in TieredPGO mode.

impDevirtualizeCall: no type available (op=LCL_VAR)

@AndyAyersMS
Copy link
Member

We don't do GDV during late devirt (which typically kicks in when the object being dispatched on is the result of a call). But we should have observed the class anyways and caught this in early devirt.

Looks like there is an early out in impDevirtualizeCall that does not consider GDV. We should fix this.

@AndyAyersMS
Copy link
Member

We should fix this.

I'll put up a PR later today.

AndyAyersMS added a commit to AndyAyersMS/runtime that referenced this issue Jul 14, 2021
Update the early bailout in impDevirtualizeCall that gives up if
the object class cannot not be determined to check if we can do
GDV (guarded devirtualization) based on profile data.

Fixes one of the cases noted in dotnet#55079.
AndyAyersMS added a commit that referenced this issue Jul 15, 2021
Update the early bailout in impDevirtualizeCall that gives up if
the object class cannot not be determined to check if we can do
GDV (guarded devirtualization) based on profile data.

Fixes one of the cases noted in #55079.
@EgorBo
Copy link
Member

EgorBo commented Jun 21, 2022

Moving to Future, my snippet was fixed by Andy while the Jakob's one is fixed with PGO - PGO applies GDV for that ToString() call and then later VN/CP eliminates the type check

@EgorBo EgorBo modified the milestones: 7.0.0, 8.0.0 Jun 21, 2022
@EgorBo EgorBo modified the milestones: 8.0.0, Future Mar 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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

4 participants