Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Do not allow generic inlining of delegate creation #102299

Merged
merged 1 commit into from
May 16, 2024

Conversation

MichalStrehovsky
Copy link
Member

Fixes #102259.

Delegate creation requires form of lookups that are hard to express across inlined generics.

This was introduced when we started doing more generic inlining in #99265. I suspected things might not be correct here but assumed we'd have test coverage for sure. Now we have test coverage.

Cc @dotnet/ilc-contrib

Fixes dotnet#102259.

Delegate creation requires form of lookups that are hard to express across inlined generics.

This was introduced when we started doing more generic inlining in dotnet#99265. I [suspected](dotnet#99265 (comment)) things might not be correct here but assumed we'd have test coverage for sure. _Now_ we have test coverage.
Copy link
Contributor

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

MichalStrehovsky added a commit to MichalStrehovsky/runtime that referenced this pull request May 16, 2024
Noticed this when working on dotnet#102299. This is bypassing the `CORINFO_LOOKUP_NOT_SUPPORTED` backout logic. Didn't actually test this, I'll let CI decide if this is a good change.
@MichalStrehovsky
Copy link
Member Author

/azp run runtime-nativeaot-outerloop

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

Copy link
Member

@EgorBo EgorBo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I presume this is NAOT specific? The test seems to be passing on CoreCLR when I run it locally

@jkotas
Copy link
Member

jkotas commented May 16, 2024

I presume this is NAOT specific? The test seems to be passing on CoreCLR when I run it locally

CoreCLR uses a slow fallback path for the complex delegates like the one used in the repro. The slow fallback path (COMDelegate::DelegateConstruct) is based on the full type loader:

coreclr!COMDelegate::DelegateConstruct [D:\a\_work\1\s\src\coreclr\vm\comdelegate.cpp @ 1593] 
 repro!Ext.WithObjectTResult[[System.__Canon, System.Private.CoreLib],[System.__Canon, System.Private.CoreLib]](System.Func`2<System.__Canon,System.__Canon>)+0x70
repro!Test102259Regression+Gen`1[[System.__Canon, System.Private.CoreLib]]..cctor()+0xee

NativeAOT does not want to depend on full type loader to create delegates.

You can think about it as NativeAOT-specific optimizations that is not implemented in CoreCLR (yet).

@MichalStrehovsky MichalStrehovsky merged commit 2c39e05 into dotnet:main May 16, 2024
124 of 134 checks passed
@MichalStrehovsky MichalStrehovsky deleted the fix102259 branch May 16, 2024 20:48
Ruihan-Yin pushed a commit to Ruihan-Yin/runtime that referenced this pull request May 30, 2024
Fixes dotnet#102259.

Delegate creation requires form of lookups that are hard to express across inlined generics.

This was introduced when we started doing more generic inlining in dotnet#99265. I [suspected](dotnet#99265 (comment)) things might not be correct here but assumed we'd have test coverage for sure. _Now_ we have test coverage.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[NativeAOT] Failed to compile WinRT project
3 participants