Make virtual delegate targets less costly

[MichalStrehovsky]

Saves 1.5% on the Stage2 app.

We make sure delegate targets are reflection visible to support Delegate.GetMethodInfo. When I originally did this work in #70198 I made a shortcut to avoid yet another node kind in the system (with a giant comment explaining what the problem is). But there's a lot of benefit in having this new node for apps with many reflection visible virtual methods.

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

---

Saves 1.5% on the Stage2 app.

We make sure delegate targets are reflection visible to support Delegate.GetMethodInfo. When I originally did this work in #70198 I made a shortcut to avoid yet another node kind in the system (with a giant comment explaining what the problem is). But there's a lot of benefit in having this new node for apps with many reflection visible virtual methods.

Cc @dotnet/ilc-contrib

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

[jkotas]

Test failure looks related:

Method `Mono.Linker.Tests.Cases.DataFlow.StaticInterfaceMethodDataflow.DamOnGenericKeepsMethod.ImplIFoo.VirtualMethod()' should have been kept\nExpected: True\nActual: False

[vitek-karas]

The test infra reacts to ReflectableMetod nodes, not the new node type. That said I don't see why the test in question would have anything marked as a delegate target.

[MichalStrehovsky]

The test that's failing is this one:

runtime/src/tools/illink/test/Mono.Linker.Tests.Cases/DataFlow/StaticInterfaceMethodDataflow.cs

Lines 22 to 55 in 19712d9

	[Kept]
	static class DamOnGenericKeepsMethod
	{
	[Kept]
	interface IFoo
	{
	[Kept]
	public static virtual void VirtualMethod () { }
	}
	[Kept]
	[KeptInterface (typeof (IFoo))]
	class ImplIFoo : IFoo
	{
	[Kept]
	public static void VirtualMethod () { }
	}
	[Kept]
	static void MethodWithDamOnType<
	[KeptAttributeAttribute (typeof (DynamicallyAccessedMembersAttribute))]
	[DynamicallyAccessedMembers (DynamicallyAccessedMemberTypes.PublicMethods)]
	T> ()
	{
	}
	[Kept]
	public static void Test ()
	{
	MethodWithDamOnType<IFoo> ();
	var _ = typeof (ImplIFoo);
	}
	}

I can't think of any reason why ImplIFoo.VirtualMethod should be kept besides Type.GetInterfaceMap. Can anyone see other way how that would be observable?

If there isn't any, I'd suggest we just disable this test on #89157 and move on with life. If that issue is ever fixed we'd need to make this marking conditional on GetInterfaceMap being called somewhere in the app because 1.5% is a lot to leave on the table.

[vitek-karas]

I think you're correct - the method is not needed.
I updated the test and also added a variation which actually calls the method to make sure it's kept.

[MichalStrehovsky]

@vitek-karas could you have a look?

[vitek-karas]

Nice - thanks!