Improve codegen for switch when cases are sealed classes

[Sergio0694]

Description

The C# compiler currently produces not really ideal codegen when using a switch statement where each case declares a variable of a given type and then uses it into that case, when the declared type is a sealed class. It looks like the compiler does a series of safe casts, and multiple checks for null. This is not needed when each type being checked is sealed. Consider this example:

public class Program
{
    public void M1(object obj)
    {
        switch (obj)
        {
            case A a: Do(a.Number); break;
            case B b: Do(b.Number); break;
            case C c: Do(c.Text); break;
        }
    }
    
    [MethodImpl(MethodImplOptions.NoInlining)]
    private static void Do(int x) { }
    
    [MethodImpl(MethodImplOptions.NoInlining)]
    private static void Do(float x) { }
    
    [MethodImpl(MethodImplOptions.NoInlining)]
    private static void Do(string x) { }
}

public sealed class A
{
    public int Number { get; }
}

public sealed class B
{
    public float Number { get; }
}

public sealed class C
{
    public string Text { get; }
}

The C# compiler currently produces the following:

public void M1(object obj)
{
    A a = obj as A;
    if (a == null)
    {
        B b = obj as B;
        if (b == null)
        {
            C c = obj as C;
            if (c != null)
            {
                Do(c.Text);
            }
        }
        else
        {
            Do(b.Number);
        }
    }
    else
    {
        Do(a.Number);
    }
}

Which could actually be rewritten in a faster way like so:

public void M1(object obj)
{
    if (obj is null) return;

    if (obj.GetType() == typeof(A))
    {
        A a = Unsafe.As<A>(obj);
        Do(a.Number);
    }
    else if (obj.GetType() == typeof(B))
    {
        B b = Unsafe.As<B>(obj);
        Do(b.Number);
    }
    else if (obj.GetType() == typeof(C))
    {
        C c = Unsafe.As<C>(obj);
        Do(c.Text);
    }
}

This does a single null check, has much faster type checks as the JIT recognizes the GetType() == typeof(T) pattern and only emits the single address check, plus the Unsafe.As call can then skip the checks once we know we have the correct type. Of course this is only a general idea, and this should just be properly written in IL directly, without those Unsafe.As calls. There does seem to be room left for improvements in this particular case though 😊

Full sharplab.io sample here.

Configuration

.NET Core 3.1, Release, on sharplab.io.

Other information

As mentioned above, this can currently be optimized manually by using the Unsafe.As method. This is not ideal though as the code is more verbose, more error prone, and I believe using generic methods can have some issues in AOT scenarios like R2R? I think I remember @GrabYourPitchforks mentioning this in the past when talking about the JIT intrinsics for GetRawArrayData in dotnet/runtime#35528.

[tannergooding]

I would think that the following would be a better pattern to emit:

    public static void M1(object obj)
    {
        if (obj is A a)
        {
            Do(a.Number);
        }
        else if (obj is B b)
        {
            Do(b.Number);
        }
        else if (obj is C c)
        {
            Do(c.Text);
        }
    }

x is T t is a newer pattern but one that will likely be much more common as time goes on and is already getting better optimizations in the JIT.
Additionally, it wouldn't depend on Unsafe.As, which is likely a bonus.

[svick]

@Sergio0694

has much faster type checks as the JIT recognizes the GetType() == typeof(T) pattern and only emits the single address check

As far as I can tell, the JIT uses the same type check in both cases, e.g.:

mov rax, 0x7ff8599bd130
cmp [rcx], rax
je short L0019

The only real difference between the two cases is that the current version has unnecessary repeated null checks.

@tannergooding

I would think that the following would be a better pattern to emit

That's pretty much what the compiler already emits and it's equivalent to the sequence of "as, null check" shown above.

Additionally, it wouldn't depend on Unsafe.As

As I understand it, this proposal wouldn't either: the compiler would emit (unverifiable) IL that performs the equivalent of Unsafe.As.

---

That being said, the only reason the proposed codegen is faster is that it avoids repeated null checks. And I think that's something that would be better handled by the JIT. And if the JIT did handle this better, it would improve the performance of both switch and x is T t.

[tannergooding]

That's pretty much what the compiler already emits and it's equivalent to the sequence of "as, null check" shown above.

You're right, this looks like sharplab just showing a different decompilation to the actual IL (which is isinst).

[Sergio0694]

@svick Thank you for chiming in and for sharing more info! And yes, this proposal was just to emit IL that would do the equivalent of that Unsafe.As trick, but without the generic calls and the dependency on those APIs (kinda like MemoryMarshal.GetArrayDataReference does).

If you say this is then only something regarding the JIT compiler, should I rephrase this issue better to clarify that and possibly this should get moved back into dotnet/runtime?
Also, I'm wondering whether this is actually just a subset of dotnet/runtime#36649 then? 🤔

[svick]

@Sergio0694

Also, I'm wondering whether this is actually just a subset of dotnet/runtime#36649 then? 🤔

I think so, yes. If the fourth case you described in that issue ("object is T value, when T is a sealed class") was fixed, then I think there would be no real performance difference between the current IL and your proposed optimization.