[mono][interp] Defer compilation in bblocks with unitialized stack #108731
+185
−49
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
|
Tagging subscribers to this area: @BrzVlad, @kotlarmilos |
This was referenced Oct 10, 2024
BrzVlad
force-pushed
the
fix-interp-bb-stack
branch
from
cc95bce to
36b9374
Compare
This was referenced Oct 15, 2024
BrzVlad
force-pushed
the
fix-interp-bb-stack
branch
from
36b9374 to
754a1a9
Compare
This was referenced Oct 21, 2024
BrzVlad
force-pushed
the
fix-interp-bb-stack
branch
from
754a1a9 to
9d8dbad
Compare
Each basic block will have an emit state, not emitted, emitting or emitted. When we reach a new basic block, we will emit code into it only if the stack state is initialized (the stack state of a bblock can be initialized either from the state of the previous bblocks, if it is fallthrough, or from branching from another bblock with initialized state). If we encounter a bblock that doesn't have the state initialized we set a flag so we will retry codegen in an attempt to emit new bblocks. Once we finish emitting code, we remove all bblocks in not emitted state.
…ed ranges Following the change to only emit code in bblocks once we reach them with an initialized stack state, we have the side effect of not processing IL code in dead bblocks. This means that offset_to_bb might actually be null for some IL offsets, so we need to iterate over following il offsets until we find a mapped bblock.
BrzVlad
force-pushed
the
fix-interp-bb-stack
branch
from
9d8dbad to
f601a5b
Compare
BrzVlad
changed the title
kotlarmilos
approved these changes
Nov 4, 2024
| g_assert (bb); | ||
| // If the bblock is detected as dead while traversing the IL code, the mapping for | ||
| // it is cleared. We can skip it. | ||
| if (!bb) |
There was a problem hiding this comment.
I’m concerned about relaxing the condition here. If bblock is null due to a bug, it might be incorrectly processed here. Could we explicitly annotate a bblock as dead instead?
There was a problem hiding this comment.
We do have a dead field for basic blocks, but the existing pattern is that dead bblocks are no longer linked to live bblocks, they are not reachable. In addition to being a different pattern, having them still exist in the td->offset_to_bb mapping turned out to complicate code in other places. This condition here is used only for exception clause ranges so I would say the scope is limited enough so we don't risk serious bugs.
This was referenced Nov 26, 2024
|
@BrzVlad looks like a big regression on wasm dotnet/perf-autofiling-issues#45939 |
BrzVlad
added a commit
to BrzVlad/runtime
that referenced
this pull request
Dec 6, 2024
Many methods in the BCL, especially hwintrins related, contain a lot of code that is detected as dead during compilation. On mono, inlining happens during IL import and a lot of optimizations are run as later passes. This exposed the issue where we have a lot of dead code bloat from inlining, with optimizations running on it. A simple solution for this problem was tracking jump counts for each bblock (dotnet#97514), which are initialized when bblocks are first created, before IL import stage. Then a small set of IL import level optimizations were added, in order to reduce the jump targets of each bblock. As we were further importing IL, if we reached a bblock with 0 jump targets, we would disable inlining into it, in order to reduce code bloat. Disabling code emit altogether was too challenging. Another limitation of this approach was that we would fail to detect dead code if it was part of a loop. The results were good however, by reducing mem usage in `System.Numerics.Tensor.Tests` from 6GB to 600MB. For an unrelated issue, the order in which we generate bblocks was redesigned in order to account for bblock stack state initialization in weird control flow scenarios (dotnet#108731). This was achieved by deferring IL import into bblock that was not yet reached from other live bblocks. A side effect of this is that we no longer generate code at all in unreachable bblocks, completely superseding the previous approach while addressing both the problems of inlining into loops or generating IR for dead IL. In the previously mentioned test suite, this further reduces the memory usage to 300MB. Remnants of the unnecessary `no_inlining` approach still lingered in the code, leading to disabling of inline optimization in some reachable code. This triggered a significant performance regression.
BrzVlad
added a commit
to BrzVlad/runtime
that referenced
this pull request
Dec 6, 2024
Many methods in the BCL, especially hwintrins related, contain a lot of code that is detected as dead during compilation. On mono, inlining happens during IL import and a lot of optimizations are run as later passes. This exposed the issue where we have a lot of dead code bloat from inlining, with optimizations later running on it. A simple solution for this problem was tracking jump counts for each bblock (dotnet#97514), which are initialized when bblocks are first created, before IL import stage. Then a small set of IL import level optimizations were added, in order to reduce the jump targets of each bblock. As we were further importing IL, if we reached a bblock with 0 jump targets, we would disable inlining into it, in order to reduce code bloat. Disabling code emit altogether was too challenging. Another limitation of this approach was that we would fail to detect dead code if it was part of a loop. The results were good however, by reducing mem usage in `System.Numerics.Tensor.Tests` from 6GB to 600MB. For an unrelated issue, the order in which we generate bblocks was redesigned in order to account for bblock stack state initialization in weird control flow scenarios (dotnet#108731). This was achieved by deferring IL import into bblocks that were not yet reached from other live bblocks. A side effect of this is that we no longer generate code at all in unreachable bblocks, completely superseding the previous approach while addressing both the problems of inlining into loops or generating IR for dead IL. In the previously mentioned test suite, this further reduced the memory usage to 300MB. Remnants of the unnecessary `no_inlining` approach still lingered in the code, leading to disabling of inline optimization in some reachable code. This triggered a significant performance regression which this PR addresses.
mikelle-rogers
pushed a commit
to mikelle-rogers/runtime
that referenced
this pull request
Dec 10, 2024
…otnet#108731) * [mono][interp] Add bblock start verbose logging * [mono][interp] Minor fixes around IL offsets with inlining * [mono][interp] Defer compilation in bblocks with unitialized stack Each basic block will have an emit state, not emitted, emitting or emitted. When we reach a new basic block, we will emit code into it only if the stack state is initialized (the stack state of a bblock can be initialized either from the state of the previous bblocks, if it is fallthrough, or from branching from another bblock with initialized state). If we encounter a bblock that doesn't have the state initialized we set a flag so we will retry codegen in an attempt to emit new bblocks. Once we finish emitting code, we remove all bblocks in not emitted state. * [mono][interp] Fix obtaining of native offsets when computing protected ranges Following the change to only emit code in bblocks once we reach them with an initialized stack state, we have the side effect of not processing IL code in dead bblocks. This means that offset_to_bb might actually be null for some IL offsets, so we need to iterate over following il offsets until we find a mapped bblock. --------- Co-authored-by: Larry Ewing <lewing@microsoft.com>
BrzVlad
added a commit
that referenced
this pull request
Dec 10, 2024
…0468) Many methods in the BCL, especially hwintrins related, contain a lot of code that is detected as dead during compilation. On mono, inlining happens during IL import and a lot of optimizations are run as later passes. This exposed the issue where we have a lot of dead code bloat from inlining, with optimizations later running on it. A simple solution for this problem was tracking jump counts for each bblock (#97514), which are initialized when bblocks are first created, before IL import stage. Then a small set of IL import level optimizations were added, in order to reduce the jump targets of each bblock. As we were further importing IL, if we reached a bblock with 0 jump targets, we would disable inlining into it, in order to reduce code bloat. Disabling code emit altogether was too challenging. Another limitation of this approach was that we would fail to detect dead code if it was part of a loop. The results were good however, by reducing mem usage in `System.Numerics.Tensor.Tests` from 6GB to 600MB. For an unrelated issue, the order in which we generate bblocks was redesigned in order to account for bblock stack state initialization in weird control flow scenarios (#108731). This was achieved by deferring IL import into bblocks that were not yet reached from other live bblocks. A side effect of this is that we no longer generate code at all in unreachable bblocks, completely superseding the previous approach while addressing both the problems of inlining into loops or generating IR for dead IL. In the previously mentioned test suite, this further reduced the memory usage to 300MB. Remnants of the unnecessary `no_inlining` approach still lingered in the code, leading to disabling of inline optimization in some reachable code. This triggered a significant performance regression which this PR addresses.
hez2010
pushed a commit
to hez2010/runtime
that referenced
this pull request
Dec 14, 2024
…net#110468) Many methods in the BCL, especially hwintrins related, contain a lot of code that is detected as dead during compilation. On mono, inlining happens during IL import and a lot of optimizations are run as later passes. This exposed the issue where we have a lot of dead code bloat from inlining, with optimizations later running on it. A simple solution for this problem was tracking jump counts for each bblock (dotnet#97514), which are initialized when bblocks are first created, before IL import stage. Then a small set of IL import level optimizations were added, in order to reduce the jump targets of each bblock. As we were further importing IL, if we reached a bblock with 0 jump targets, we would disable inlining into it, in order to reduce code bloat. Disabling code emit altogether was too challenging. Another limitation of this approach was that we would fail to detect dead code if it was part of a loop. The results were good however, by reducing mem usage in `System.Numerics.Tensor.Tests` from 6GB to 600MB. For an unrelated issue, the order in which we generate bblocks was redesigned in order to account for bblock stack state initialization in weird control flow scenarios (dotnet#108731). This was achieved by deferring IL import into bblocks that were not yet reached from other live bblocks. A side effect of this is that we no longer generate code at all in unreachable bblocks, completely superseding the previous approach while addressing both the problems of inlining into loops or generating IR for dead IL. In the previously mentioned test suite, this further reduced the memory usage to 300MB. Remnants of the unnecessary `no_inlining` approach still lingered in the code, leading to disabling of inline optimization in some reachable code. This triggered a significant performance regression which this PR addresses.
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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.
Each basic block will have an emit state, not emitted, emitting or emitted. When we reach a new basic block, we will emit code into it only if the stack state is initialized (the stack state of a bblock can be initialized either from the state of the previous bblocks, if it is fallthrough, or from branching from another bblock with initialized state). If we encounter a bblock that doesn't have the state initialized we set a flag so we will retry codegen in an attempt to emit new bblocks.
Once we finish emitting code, we remove all bblocks in not emitted state.
Before this change, when encountering a bblock with unitialized stack, we assumed by chance that it had an empty stack, which is incorrect according to the spec. Also, in some cases we could simply crash, even if the block was indeed having an empty stack.