Propagate static readonly loads by value during inlining

[MichalPetryka]

Makes the JIT propagate invariant loads without side effects when inlining, preventing the JIT from spilling them.

Contains a hack to avoid static readonly args boosting inliner profitability.

[dotnet-policy-service]

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

[MichalPetryka]

@MihuBot

[MichalPetryka]

@MihuBot

[MichalPetryka]

@MihuBot

[MichalPetryka]

@MihuBot

[MichalPetryka]

Okay, I looked into what's going on with inliner inlining more stuff here, apparently the reduced spilling on static readonly args which causes less locals to be used makes it reduce the profitability multiplier less which causes more inlining.
Another interesting part is that the JIT now sees loads of fields from static readonlys as side-effect free.

[MichalPetryka]

@MihuBot -dependsOn 109716

[JulieLeeMSFT]

@AndyAyersMS will review this PR.

[AndyAyersMS]

Can you add a comment up top to explain the motivation for this change?

Also, please help me understand why we should do this during inlining. The main goal of inlining's forward sub is to recognize further inlining opportunities and/or get a better handle on the cost of the inlinee.

[MichalPetryka]

Can you add a comment up top to explain the motivation for this change?

Also, please help me understand why we should do this during inlining. The main goal of inlining's forward sub is to recognize further inlining opportunities and/or get a better handle on the cost of the inlinee.

The goal here was to let #109679 work across methods, but this leads to bad diffs from duplicated loads that CSE can't handle. I opened #109715 as an alternative to limit it to delegates only cause I'm not sure if there's any way to block the bad cases here...

[AndyAyersMS]

Can you add a comment up top to explain the motivation for this change?
Also, please help me understand why we should do this during inlining. The main goal of inlining's forward sub is to recognize further inlining opportunities and/or get a better handle on the cost of the inlinee.

The goal here was to let #109679 work across methods, but this leads to bad diffs from duplicated loads that CSE can't handle. I opened #109715 as an alternative to limit it to delegates only cause I'm not sure if there's any way to block the bad cases here...

I think we should approach #109679 differently. Instead of trying to pull all this off during importation and inlining, I would like to see us focus on refactoring the inliner so it can be invoked as a utility in later phases and then rely on normal forward propagation of facts to enable new inlining in cases like these.

I realize that:

• what I'm proposing here is to solve a harder problem in order to make this one easier;
• I'm not entirely sure yet how to go about the necessary inliner refactoring;
• I have reasons to believe it will be somewhat tricky;
• I likely won't have time to seriously consider or guide anyone through it during .NET 10

So this would likely mean shelving all this work until sometime later next year.

If you want to keep working in this area a good starter project might be to move the delegate invoke expansion earlier in the jit; that might be enough to unblock escape analysis of the delegate in some cases.

I might be willing to reconsider if you strongly feel that we should consider following through on your current plans and can show some compelling data on the improvements this could bring.

[MichalPetryka]

Can you add a comment up top to explain the motivation for this change?
Also, please help me understand why we should do this during inlining. The main goal of inlining's forward sub is to recognize further inlining opportunities and/or get a better handle on the cost of the inlinee.

The goal here was to let #109679 work across methods, but this leads to bad diffs from duplicated loads that CSE can't handle. I opened #109715 as an alternative to limit it to delegates only cause I'm not sure if there's any way to block the bad cases here...

I think we should approach #109679 differently. Instead of trying to pull all this off during importation and inlining, I would like to see us focus on refactoring the inliner so it can be invoked as a utility in later phases and then rely on normal forward propagation of facts to enable new inlining in cases like these.

I realize that:

• what I'm proposing here is to solve a harder problem in order to make this one easier;
• I'm not entirely sure yet how to go about the necessary inliner refactoring;
• I have reasons to believe it will be somewhat tricky;
• I likely won't have time to seriously consider or guide anyone through it during .NET 10

So this would likely mean shelving all this work until sometime later next year.

If you want to keep working in this area a good starter project might be to move the delegate invoke expansion earlier in the jit; that might be enough to unblock escape analysis of the delegate in some cases.

I might be willing to reconsider if you strongly feel that we should consider following through on your current plans and can show some compelling data on the improvements this could bring.

Could we move all discussion regarding #109679 to its comments? I'd prefer to reply on the PR in question.

[AndyAyersMS]

I'm still not seeing why we should take this PR. Aside from helping with the delegate issue, is there any other consideration?

[AndyAyersMS]

@MichalPetryka I am planning on closing this soon, but wanted to give you one last opportunity to argue in favor.

[dotnet-policy-service]

This pull request has been automatically marked no-recent-activity because it has not had any activity for 14 days. It will be closed if no further activity occurs within 14 more days. Any new comment (by anyone, not necessarily the author) will remove no-recent-activity.

[dotnet-policy-service]

This pull request will now be closed since it had been marked no-recent-activity but received no further activity in the past 14 days. It is still possible to reopen or comment on the pull request, but please note that it will be locked if it remains inactive for another 30 days.