Dead code elimination for if (typeof(T).IsValueType)

[MichalStrehovsky]

@stephentoub found out that for following code:

using System.Buffers;

Foo<Bar>();

static T[] Foo<T>()
{
    if (typeof(T).IsValueType)
    {
        return ArrayPool<T>.Shared.Rent(42);
    }
    return null!;
}

class Bar {}

We end up generating ArrayPools of Bar even though it's obviously never reachable. The problem is architectural:

• We run a whole program analysis phase that tries to figure out things like generic dictionary layouts so that later, in code generation phase, we can inline offsets into generic dictionaries into codegen.
• For the above code, whole program analysis decides that the dictionary layout of Foo<__Canon> needs a slot for ArrayPool<!0>.
• Codegen then optimizes out the IsValueType branch because __Canon is never a valuetype. But we already allocated the dictionary slot and will fill it out, even though it ends up unused due to the optimization.

We're going to run into issues like this until #83021 is addressed. Whole program analysis cannot currently assume a certain optimization happens because we don't know whether RyuJIT will do it. The only way we can "optimize" during whole program analysis is if we rewrite IL and give RyuJIT no saying in whether to do an optimization or not. Rewriting the IL is not great because it e.g. causes PGO data to not match. I don't like doing it, but there's nothing else we can do.

This change extends dead block elimination to understand typeof(X).IsValueType. If we recognize a branch is reachable under this condition, we evaluate whether this is true or false and replace the basic block with nops.

Cc @dotnet/ilc-contrib

[ghost]

Tagging subscribers to this area: @agocke, @MichalStrehovsky, @jkotas
See info in area-owners.md if you want to be subscribed.

Issue Details

---

@stephentoub found out that for following code:

using System.Buffers;

Foo<Bar>();

static T[] Foo<T>()
{
    if (typeof(T).IsValueType)
    {
        return ArrayPool<T>.Shared.Rent(42);
    }
    return null!;
}

class Bar {}

We end up generating ArrayPools of Bar even though it's obviously never reachable. The problem is architectural:

• We run a whole program analysis phase that tries to figure out things like generic dictionary layouts so that later, in code generation phase, we can inline offsets into generic dictionaries into codegen.
• For the above code, whole program analysis decides that the dictionary layout of Foo<__Canon> needs a slot for ArrayPool<!0>.
• Codegen then optimizes out the IsValueType branch because __Canon is never a valuetype. But we already allocated the dictionary slot and will fill it out, even though it ends up unused due to the optimization.

We're going to run into issues like this until #83021 is addressed. Whole program analysis cannot currently assume a certain optimization happens because we don't know whether RyuJIT will do it. The only way we can "optimize" during whole program analysis is if we rewrite IL and give RyuJIT no saying in whether to do an optimization or not. Rewriting the IL is not great because it e.g. causes PGO data to not match. I don't like doing it, but there's nothing else we can do.

This change extends dead block elimination to understand typeof(X).IsValueType. If we recognize a branch is reachable under this condition, we evaluate whether this is true or false and replace the basic block with nops.

Cc @dotnet/ilc-contrib

Author:	MichalStrehovsky
Assignees:	-
Labels:	area-NativeAOT-coreclr
Milestone:	-

[jkotas]

Do we need to check that there are no jump targets in the sequence?

[MichalStrehovsky]

ReadLdToken and ReadGetTypeFromHandle already do that check

[jkotas]

We check mdType.Context.SystemModule in the existing case a few lines above. Do we need to do the same here?

It would be nice if both checks use the same style.

[MichalStrehovsky]

I had that, then I learned {..., Module: method.Context.SystemModule } doesn't compile, became frustrated at how useless the pattern matching in C# is, and forgot. Fixed.

[jkotas]

method.IsIntrinsic can be de-duplicated with the existing case above.

[MichalStrehovsky]

We need this to land in the else branch all the way below if it's an intrinsic but not the one we recognize.

[jkotas]

Thanks

[stephentoub]

Thanks for following up on it.