JIT: consider using new temp to capture improved type from an optimized cast

[AndyAyersMS]

dotnet/coreclr#14420 will optimize sucessful ISINST(x) orCASTCLASS(x) to be simply x. However the successful cast may give us "better" type information about x than we had before, and right now we lose that information.

We attempt this type improvement for some of the unoptimized cases when the result is produced in a temp, see the last few lines of impCastClassOrIsInstToTree. However blindly setting the type to the cast type may not always be better. For instance casting from a known class to an interface type isn't really providing much in the way of useful information.

So we should only do this sort of thing when the target type is a class, and we've cast from supertype to subtype, or from an interface to a class type.

category:cq
theme:importer
skill-level:expert
cost:small

[AndyAyersMS]

Considering for 2.1.

[AndyAyersMS]

Still interested in this but likely not happening in time for 2.1. So will push it back to Future and reconsider once we're scouting what should go into 2.2 or whatever it is that comes next.

[GrabYourPitchforks]

FYI I'm running into this in my code. I have a scenario where I want to know if a given object's type is exactly UTF8Encoding (not a derived class), and if so then perform the cast. While the JIT does optimize the pattern if (someObject.GetType() == typeof(Foo)), it does not optimize the cast (Foo)someObject immediately following.

Example:

private static UTF8Encoding AsUTF8Encoding(Encoding encoding)
{
    // input parameter is already known to be not-null
    if (encoding.GetType() == typeof(UTF8Encoding))
    {
        return (UTF8Encoding)encoding;
    }
    return null;
}

Current codegen:

    L0000: sub rsp, 0x28
    L0004: mov rdx, 0x7ff8f3db8360
    L000e: cmp [rcx], rdx
    L0011: jnz L003d
    L0013: mov [rsp+0x30], rcx
    L0018: mov rax, rcx
    L001b: mov rdx, 0x7ff8f3db8360
    L0025: cmp [rax], rdx
    L0028: jz L0037
    L002a: mov rcx, rdx
    L002d: mov rdx, [rsp+0x30]
    L0032: call 0x7ff953813460
    L0037: nop
    L0038: add rsp, 0x28
    L003c: ret
    L003d: xor eax, eax
    L003f: add rsp, 0x28
    L0043: ret

Expected codegen:

xor eax, eax
mov rdx, 0x7ff8f3db8360
cmp [rcx], rdx
jnz <past_mov_instruction_below>
mov rax, rcx
ret

[AndyAyersMS]

Tricky to pull this off in the importer.

I wonder why assertion prop can't get these cases. Need to drill in and find out why.

[AndyAyersMS]

Assertion prop should indeed get this. I have a partial fix. With it, the helper call goes away, but not the upstream redundant exact check.

[AndyAyersMS]

Diff on example like above:

-; Lcl frame size = 40
+; Lcl frame size = 0

 G_M62121_IG01:
-       sub      rsp, 40

 G_M62121_IG02:
-       mov      rdx, 0xD1FFAB1E
-       cmp      qword ptr [rcx], rdx
-       jne      SHORT G_M62121_IG07
+       mov      rax, 0xD1FFAB1E
+       cmp      qword ptr [rcx], rax
+       jne      SHORT G_M62121_IG06

 G_M62121_IG03:         ;; bbWeight=0.50
-       mov      gword ptr [rsp+30H], rcx
        mov      rax, rcx
        mov      rdx, 0xD1FFAB1E
        cmp      qword ptr [rax], rdx
        je       SHORT G_M62121_IG05

 G_M62121_IG04:         ;; bbWeight=0.12
-       mov      rcx, rdx
-       mov      rdx, gword ptr [rsp+30H]
-       call     CORINFO_HELP_CHKCASTCLASS_SPECIAL
+       mov      rax, rcx

 G_M62121_IG05:         ;; bbWeight=0.50
-       nop
-
-G_M62121_IG06:         ;; bbWeight=0.50
-       add      rsp, 40
        ret

-G_M62121_IG07:         ;; bbWeight=0.50
+G_M62121_IG06:         ;; bbWeight=0.50
        xor      rax, rax

-G_M62121_IG08:         ;; bbWeight=0.50
-       add      rsp, 40
+G_M62121_IG07:         ;; bbWeight=0.50
        ret

-; Total bytes of code 68, prolog size 4, perf score 10.30, (MethodHash=77b00d56) for method X:AsD(ref):ref
+; Total bytes of code 40, prolog size 0, perf score 7.25, (MethodHash=77b00d56) for method X:AsD(ref):ref

For the redundant upstream check, seems like we could generate assertions for JTRUE(EQ,IND(x),K) if x is TYP_REF and the offset is zero.

[AndyAyersMS]

Codegen after dotnet/coreclr#27397 is:

       mov      rax, 0xD1FFAB1E
       cmp      qword ptr [rcx], rax
       jne      SHORT G_M62121_IG05
       mov      rax, rcx
       ret      
G_M62121_IG05:
       xor      rax, rax
       ret 

So just the one exact test.

[benaadams]

Aside; will this also apply to generics? (though is conversion probably works too now with C#7+)

e.g.

// T encoding
if (typeof(T) == typeof(MyClass))
{
    ((MyClass)(object)encoding).Method();
}

dotnet/csharplang#905 (comment)

Specific example is https://github.com/aspnet/AspNetCore/blob/master/src/Servers/Kestrel/Core/src/Internal/Http/HttpProtocol.Generated.cs#L395-L517:

TFeature IFeatureCollection.Get<TFeature>()
{
    TFeature feature = default;
    if (typeof(TFeature) == typeof(IHttpRequestFeature))
    {
        feature = (TFeature)_currentIHttpRequestFeature;
    }
    else if (typeof(TFeature) == typeof(IHttpResponseFeature))
    {
        feature = (TFeature)_currentIHttpResponseFeature;
    }
    else if (typeof(TFeature) == typeof(IHttpResponseBodyFeature))
    {
        feature = (TFeature)_currentIHttpResponseBodyFeature;
    }
    else if (typeof(TFeature) == typeof(IRequestBodyPipeFeature))
    {
        feature = (TFeature)_currentIRequestBodyPipeFeature;
    }
    else if (typeof(TFeature) == typeof(IHttpRequestIdentifierFeature))
    {
        feature = (TFeature)_currentIHttpRequestIdentifierFeature;
    }
...

[AndyAyersMS]

For value type instantiations of T, it should optimize. For ref type Ts, it won't.

Let me look at ASP.net diffs...

[benaadams]

Oh, nvm. Its tested the type, not if the cast is then valid.

[benaadams]

What would be valid is casting a value of type T and using it; but you'd probably do this rather than checking typeof(T)

// T value
if (value is MyClass specificValue)
{
     specificValue.MyMethod();
} 

[GrabYourPitchforks]

My very particular scenario is that I have the following type hierarchy, where I'm comparing against one of two types:

public abstract class SomeAbstractClass { }
public class SomeConcreteClass : SomeAbstractClass { }
internal sealed class SomeSealedClass : SomeConcreteClass { }

public static void DoSomething(SomeAbstractClass obj)
{
    if (obj.GetType() == typeof(SomeConcreteClass) || obj.GetType() == typeof(SomeSealedClass))
    {
        SomeConcreteClass castObj = (SomeConcreteClass)obj;
        // use castObj here
    }
}

In this scenario I want an exact type match against one of those two types, so I don't use if (obj is SomeConcreteClass castObj) { ... }. The reason for this is that a third party may have subclassed SomeConcreteClass and overridden the virtual methods to have unexpected behaviors. So I want to test for an exact type match against the two implementations I know the behavior of.

[benaadams]

Inverse GetType() test is used in StreamReader

https://github.com/dotnet/coreclr/blob/cea2bba3c295c5280d01900f7ae5492951ad8995/src/System.Private.CoreLib/shared/System/IO/StreamReader.cs#L1193-L1200

So that could "sharpen" any override calls that happen after because the exact type is known?

[AndyAyersMS]

So that could "sharpen" any override calls

It could, but the the type sharpening in dotnet/coreclr#27397 happens during optimization, which is in the middle of the phase pipeline. Today we only devirtualize early in the jit phase pipeline since we are mainly looking to unblock inlining.

So there's still see an interface dispatch below the negative test:

       mov      edx, dword ptr [rsp+44H]
       movsx    rdx, dx
       mov      rcx, rbx
       mov      r11, 0xD1FFAB1E
       cmp      dword ptr [rcx], ecx
       call     [IValueTaskSource`1:GetStatus(short):int:this]

It might be viable to do one more late devirt attempt during optimization, though it probably wouldn't be as general.

[AndyAyersMS]

@GrabYourPitchforks in your case there will still be a redundant type test at the cast. Today the jit is not able to reason about multiple types reaching a use, and is not prepared to specialize/duplicate the code to undo the loss of information at the control flow join. So you'll see something like:

G_M31063_IG02:
       48BA703487BBFB7F0000 mov      rdx, 0x7FFBBB873470
       483911               cmp      qword ptr [rcx], rdx
       740F                 je       SHORT G_M31063_IG04

G_M31063_IG03:          ;; bbWeight=0.50
       48BA283687BBFB7F0000 mov      rdx, 0x7FFBBB873628
       483911               cmp      qword ptr [rcx], rdx
       7537                 jne      SHORT G_M31063_IG08

G_M31063_IG04:          ;; bbWeight=0.50
       48894C2430           mov      gword ptr [rsp+30H], rcx
       488BC1               mov      rax, rcx
       48BA703487BBFB7F0000 mov      rdx, 0x7FFBBB873470
       483910               cmp      qword ptr [rax], rdx
       740D                 je       SHORT G_M31063_IG06

G_M31063_IG05:          ;; bbWeight=0.12
       488BCA               mov      rcx, rdx
       488B542430           mov      rdx, gword ptr [rsp+30H]
       E89A8F805F           call     CORINFO_HELP_CHKCASTCLASS_SPECIAL

If the code sequence that uses castObj is small and there is benefit from knowing the type you might consider doing this duplication in source:

public static int DoSomething2(SomeAbstractClass obj)
{
    if (obj.GetType() == typeof(SomeConcreteClass))
    {
        SomeConcreteClass castObj = (SomeConcreteClass)obj;
        ...
    }
    else if (obj.GetType() == typeof(SomeSealedClass))
    {
        SomeSealedClass castObj = (SomeSealedClass)obj;
        ...
    }

We don't do general dataflow on type information, in part because we'd have to call back into the runtime repeatedly to ask it about how types relate to one another.

[GrabYourPitchforks]

That sounds like a reasonable compromise - thanks for the tip. :)