Fix branch removal in compiler generated code #3088
Merged
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
Changes to processing of compiler generated methods lead to a state where we don't call constant prop and branch removal in all cases before we mark instructions of the method. This can lead to overmarking This change fixes this by making sure that the branch removal executes on the method in all cases before we mark instructions of the method. To make the code simple I added a hashset to prevent multiple optimization runs on the same method. Technically this can be guaranteed by the caller, but it's really complex and very fragile. Added tests. Note that there's still a whole in analysis of compiler generated code around state machines, see dotnet#3087
sbomer
reviewed
Oct 27, 2022
…e constant prop/branch removal happened on the method. This does have one possibly negative impact: the issue described in dotnet#2937 is now consistent and happens always. Basically if there's a local function which is going to be removed due to branch removal and if the body of that method contains code which produces a warning due to generic parameter validation, such warning will always be generated even though it's "dead" code and even if it's suppressed via RUC or similar. Basically in such case the analysis can't figure out to which method the local function belongs (since the call site has been removed).
|
I reworked this change to guard against accesses to Body and make sure that const. prop/branch removal happens always. This does have one negative effect: #2937 is not consistent and happens in all cases - meaning if the callsite to a local function is removed, we can't figure out to which method the local function belongs and may incorrectly report some warnings. Personally I prefer the consistent behavior (which is not order dependent anymore) - even if it's incorrect. We can use #2937 to track ideas on how to fix the problem. |
vitek-karas
commented
Oct 31, 2022
vitek-karas
commented
Oct 31, 2022
|
I measured perf of this change (using the sfx project from runtime) and there's no visible regression (although locally the noise is rather high). |
jtschuster
approved these changes
Oct 31, 2022
| @@ -699,7 +702,7 @@ protected virtual void Scan (MethodBody methodBody, ref InterproceduralState int | |||
| } | |||
| } | |||
|
|
|||
| private static void ScanExceptionInformation (Dictionary<int, Stack<StackSlot>> knownStacks, MethodBody methodBody) | |||
| private static void ScanExceptionInformation (Dictionary<int, Stack<StackSlot>> knownStacks, MethodBodyInstructionsProvider.ProcessedMethodBody methodBody) | |||
There was a problem hiding this comment.
Just to make sure I understand, this needs to be a ProcessedMethodBody because it's static, right? And the other instance methods we are just assuming we pass the ProcessedMethodBody.Body from Scan?
There was a problem hiding this comment.
I don't think I did anything with static/instance methods on this class. The idea is that Scan gets the processed method from the outside and it passes it to all of the helpers here - including this one.
Sorry, I probably don't understand the question.
sbomer
approved these changes
Nov 1, 2022
marek-safar
reviewed
Nov 1, 2022
Basically just refactored the helper class into a separate MethodIL and moved the logic onto LinkContext. The rest is just renames and cleanup.
marek-safar
reviewed
Nov 1, 2022
tlakollo
pushed a commit
to dotnet/runtime
that referenced
this pull request
Nov 4, 2022
Changes to processing of compiler generated methods lead to a state where we don't call constant prop and branch removal in all cases before we mark instructions of the method. This can lead to overmarking This change fixes this by making sure that the branch removal executes on the method in all cases before we mark instructions of the method. The change guarantees that all accesses to Body are after the constant prop/branch removal happened on the method. This does have one possibly negative impact: the issue described in dotnet/linker#2937 is now consistent and happens always. Added tests. Note that there's still a whole in analysis of compiler generated code around state machines, see dotnet/linker#3087 Basically if there's a local function which is going to be removed due to branch removal and if the body of that method contains code which produces a warning due to generic parameter validation, such warning will always be generated even though it's "dead" code and even if it's suppressed via RUC or similar. In such case the analysis can't figure out to which method the local function belongs (since the call site has been removed). Commit migrated from dotnet/linker@e502e72
agocke
pushed a commit
to dotnet/runtime
that referenced
this pull request
Nov 16, 2022
Changes to processing of compiler generated methods lead to a state where we don't call constant prop and branch removal in all cases before we mark instructions of the method. This can lead to overmarking This change fixes this by making sure that the branch removal executes on the method in all cases before we mark instructions of the method. The change guarantees that all accesses to Body are after the constant prop/branch removal happened on the method. This does have one possibly negative impact: the issue described in dotnet/linker#2937 is now consistent and happens always. Added tests. Note that there's still a whole in analysis of compiler generated code around state machines, see dotnet/linker#3087 Basically if there's a local function which is going to be removed due to branch removal and if the body of that method contains code which produces a warning due to generic parameter validation, such warning will always be generated even though it's "dead" code and even if it's suppressed via RUC or similar. In such case the analysis can't figure out to which method the local function belongs (since the call site has been removed). Commit migrated from dotnet/linker@e502e72
vitek-karas
added a commit
to vitek-karas/linker
that referenced
this pull request
Dec 13, 2022
Changes to processing of compiler generated methods lead to a state where we don't call constant prop and branch removal in all cases before we mark instructions of the method. This can lead to overmarking This change fixes this by making sure that the branch removal executes on the method in all cases before we mark instructions of the method. The change guarantees that all accesses to Body are after the constant prop/branch removal happened on the method. This does have one possibly negative impact: the issue described in dotnet#2937 is now consistent and happens always. Added tests. Note that there's still a whole in analysis of compiler generated code around state machines, see dotnet#3087 Basically if there's a local function which is going to be removed due to branch removal and if the body of that method contains code which produces a warning due to generic parameter validation, such warning will always be generated even though it's "dead" code and even if it's suppressed via RUC or similar. In such case the analysis can't figure out to which method the local function belongs (since the call site has been removed).
sbomer
pushed a commit
that referenced
this pull request
Jan 18, 2023
* Fix branch removal in compiler generated code (#3088) Changes to processing of compiler generated methods lead to a state where we don't call constant prop and branch removal in all cases before we mark instructions of the method. This can lead to overmarking This change fixes this by making sure that the branch removal executes on the method in all cases before we mark instructions of the method. The change guarantees that all accesses to Body are after the constant prop/branch removal happened on the method. This does have one possibly negative impact: the issue described in #2937 is now consistent and happens always. Added tests. Note that there's still a whole in analysis of compiler generated code around state machines, see #3087 Basically if there's a local function which is going to be removed due to branch removal and if the body of that method contains code which produces a warning due to generic parameter validation, such warning will always be generated even though it's "dead" code and even if it's suppressed via RUC or similar. In such case the analysis can't figure out to which method the local function belongs (since the call site has been removed). * PR feedback
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.
Changes to processing of compiler generated methods lead to a state where we don't call constant prop and branch removal in all cases before we mark instructions of the method. This can lead to overmarking
This change fixes this by making sure that the branch removal executes on the method in all cases before we mark instructions of the method.
To make the code simple I added a hashset to prevent multiple optimization runs on the same method. Technically this can be guaranteed by the caller, but it's really complex and very fragile.
Added tests.
Note that there's still a whole in analysis of compiler generated code around state machines, see #3087