Allow devirt into abstract classes if we saw a non-abstract child #108379
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
We avoid devirtualizing into abstract classes because whole program view might have optimized away the method bodies and devirtualizing them doesn't lead to anything good. However, if the whole program view had a non-abstract child of this, we can no longer optimize this out and devirtualization should be fine. Fixes issue encountered in dotnet#108153 (comment)
|
Tagging subscribers to this area: @agocke, @MichalStrehovsky, @jkotas |
3 tasks
|
/azp run runtime-nativeaot-outerloop |
|
Azure Pipelines successfully started running 1 pipeline(s). |
jkotas
approved these changes
Sep 30, 2024
src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/ILScanner.cs
Outdated
Show resolved
Hide resolved
src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/ILScanner.cs
Outdated
Show resolved
Hide resolved
MichalStrehovsky
added
the
NO-MERGE
|
The failures in outerloop look like this exposed a pre-existing bug, so marking no merge. |
|
/azp run runtime-nativeaot-outerloop |
|
Azure Pipelines successfully started running 1 pipeline(s). |
MichalStrehovsky
added a commit
to MichalStrehovsky/runtime
that referenced
this pull request
Oct 1, 2024
This code was trying to answer question: "Was this method overriden by something else in a more derived class"? It was walking the base hierarchy in canonical form, but that was leading to methods not resolving at all. The fix is to walk the non-canonical hierarchy and canonicalize after we resolved everything. I ran into this in dotnet#108379 that unlocked more whole program devirtualization and `StringSearchValuesBase` is in this shape.
MichalStrehovsky
added a commit
that referenced
this pull request
Oct 1, 2024
This code was trying to answer question: "Was this method overriden by something else in a more derived class"? It was walking the base hierarchy in canonical form, but that was leading to methods not resolving at all. The fix is to walk the non-canonical hierarchy and canonicalize after we resolved everything. I ran into this in #108379 that unlocked more whole program devirtualization and `StringSearchValuesBase` is in this shape.
|
/azp run runtime-nativeaot-outerloop |
|
Azure Pipelines successfully started running 1 pipeline(s). |
MichalStrehovsky
added a commit
to MichalStrehovsky/runtime
that referenced
this pull request
Oct 2, 2024
This code was trying to answer question: "Was this method overriden by something else in a more derived class"? It was walking the base hierarchy in canonical form, but that was leading to methods not resolving at all. The fix is to walk the non-canonical hierarchy and canonicalize after we resolved everything. I ran into this in dotnet#108379 that unlocked more whole program devirtualization and `StringSearchValuesBase` is in this shape.
|
/azp run runtime-nativeaot-outerloop |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
CI found more places that needed compensation for this new relaxation:
|
|
/azp run runtime-nativeaot-outerloop |
|
Azure Pipelines successfully started running 1 pipeline(s). |
3 tasks
sirntar
pushed a commit
to sirntar/runtime
that referenced
this pull request
Oct 3, 2024
This code was trying to answer question: "Was this method overriden by something else in a more derived class"? It was walking the base hierarchy in canonical form, but that was leading to methods not resolving at all. The fix is to walk the non-canonical hierarchy and canonicalize after we resolved everything. I ran into this in dotnet#108379 that unlocked more whole program devirtualization and `StringSearchValuesBase` is in this shape.
sirntar
pushed a commit
to sirntar/runtime
that referenced
this pull request
Oct 3, 2024
…tnet#108379) We avoid devirtualizing into abstract classes because whole program view might have optimized away the method bodies and devirtualizing them doesn't lead to anything good. However, if the whole program view had a non-abstract child of this, we can no longer optimize this out and devirtualization should be fine. Fixes issue encountered in dotnet#108153 (comment)
jeffschwMSFT
added a commit
that referenced
this pull request
Oct 3, 2024
…108470) This code was trying to answer question: "Was this method overriden by something else in a more derived class"? It was walking the base hierarchy in canonical form, but that was leading to methods not resolving at all. The fix is to walk the non-canonical hierarchy and canonicalize after we resolved everything. I ran into this in #108379 that unlocked more whole program devirtualization and `StringSearchValuesBase` is in this shape. Co-authored-by: Jeff Schwartz <jeffschw@microsoft.com>
lambdageek
pushed a commit
to lambdageek/runtime
that referenced
this pull request
Oct 3, 2024
This code was trying to answer question: "Was this method overriden by something else in a more derived class"? It was walking the base hierarchy in canonical form, but that was leading to methods not resolving at all. The fix is to walk the non-canonical hierarchy and canonicalize after we resolved everything. I ran into this in dotnet#108379 that unlocked more whole program devirtualization and `StringSearchValuesBase` is in this shape.
lambdageek
pushed a commit
to lambdageek/runtime
that referenced
this pull request
Oct 3, 2024
…tnet#108379) We avoid devirtualizing into abstract classes because whole program view might have optimized away the method bodies and devirtualizing them doesn't lead to anything good. However, if the whole program view had a non-abstract child of this, we can no longer optimize this out and devirtualization should be fine. Fixes issue encountered in dotnet#108153 (comment)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
We avoid devirtualizing into abstract classes because whole program view might have optimized away the method bodies and devirtualizing them doesn't lead to anything good.
However, if the whole program view had a non-abstract child of the abstract class, whole program can no longer optimize this out and devirtualization should be fine.
Fixes issue encountered in #108153 (comment)
Cc @dotnet/ilc-contrib