Delete useless delegate virtual methods

[MichalStrehovsky]

The split between Delegate and MulticastDelegate is a wart that likely has some history behind it. Types that inherit from Delegate directly would not be considered delegates by the runtime. They need to inherit MulticastDelegate. I can't find a reason why we'd need some useless base implementation of these methods that immediately gets overriden in MulticastDelegate. This deletes the useless base implementation and moves the useful implementation from MulticastDelegate to Delegate.

This along with #97951 saves ~40 bytes per delegate MethodTable because the virtual methods can now be devirtualized or placed into the sealed vtable. We might be able to do even more since technically sealed virtuals could be reshuffled after the codegen phase and slots eliminated then.

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

---

The split between Delegate and MulticastDelegate is a wart that likely has some history behind it. Types that inherit from Delegate directly would not be considered delegates by the runtime. They need to inherit MulticastDelegate. I can't find a reason why we'd need some useless base implementation of these methods that immediately gets overriden in MulticastDelegate. This deletes the useless base implementation and moves the useful implementation from MulticastDelegatetoDelegate`.

This along with #97951 saves ~40 bytes per delegate MethodTable because the virtual methods can now be devirtualized or placed into the sealed vtable. We might be able to do even more since technically sealed virtuals could be reshuffled after the codegen phase and slots eliminated then.

Cc @dotnet/ilc-contrib

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

[jkotas]

LGTM otherwise

[jkotas]

Can this file be moved under libraries and shared between runtimes now?

[MichalPetryka]

Suggested change

	if (d2 is null)
	{
	return d1 is null;
	}
	return ReferenceEquals(d2, d1) ? true : d2.Equals((object?)d1);
	if (ReferenceEquals(d1, d2))
	{
	return true;
	}
	return d2 is not null && d2.Equals((object?)d1);

Wouldn't this be better?

[jkotas]

"Test d2 first to allow branch elimination when inlined for null checks (== null) so it can become a simple test" comment above explains why the code is written the way it is written.

[MichalPetryka]

"Test d2 first to allow branch elimination when inlined for null checks (== null) so it can become a simple test" comment above explains why the code is written the way it is written.

ReferenceEquals lets it become a simple test too though with a constant null?

G_M4196_IG01:  ;; offset=0x0000
						;; size=0 bbWeight=1 PerfScore 0.00
G_M4196_IG02:  ;; offset=0x0000
       33C0                 xor      eax, eax
       4885C9               test     rcx, rcx
       0F94C0               sete     al
						;; size=8 bbWeight=1 PerfScore 1.50
G_M4196_IG03:  ;; offset=0x0008
       C3                   ret      
						;; size=1 bbWeight=1 PerfScore 1.00

[jkotas]

Yes, this looks better. Would you like to submit a PR with this improvement?

This pattern came from dotnet/coreclr#21765 that applied this pattern in multiple places (the pattern was later modified in dotnet/coreclr#21829 to avoid a potential breaking change). All these places can use the same improvement.