Prevent pops from sinking in SimplifyLocals #2885
Merged
Conversation
This file contains hidden or 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
This prevents `exnref.pop`s from being sinked and separated from
`catch`. For example,
```wast
(try
(do)
(catch
(local.set $0 (exnref.pop))
(call $foo
(i32.const 3)
(local.get $0)
)
)
)
```
Here, if we sink `exnref.pop` to remove `local.set $0` and
`local.get $0`, it becomes this:
```wast
(try
(do)
(catch
(nop)
(call $foo
(i32.const 3)
(exnref.pop)
)
)
)
```
This move was possible because `i32.const 3` does not have any side
effects. But this is incorrect because now `exnref.pop` does not follow
right after `catch`.
To prevent this, this patch checks this case in `canSink` in
SimplifyLocals. When we encountered a similar case in CodeFolding, we
prevented every expression that contains `Pop` anywhere in it from being
moved, which was too conservative. This adds `danglingPop` property in
`EffectAnalyzer`, so that only pops that are not enclosed within a
`catch` count as 'dangling pops` and we only prevent those pops from
being moved or sinked.
kripken
reviewed
Jun 1, 2020
kripken
approved these changes
Jun 2, 2020
src/passes/SimplifyLocals.cpp
Outdated
| @@ -411,6 +411,13 @@ struct SimplifyLocals | |||
| if (set->isTee()) { | |||
| return false; | |||
| } | |||
| // We cannot move expressions containing exnref.pops that are not enclosed | |||
| // in 'catch', because 'exnref.pop' should follow right after 'catch'. | |||
| if (EffectAnalyzer( | |||
There was a problem hiding this comment.
how about checking for the exceptions feature here, to avoid computing all side effects when it is disabled?
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.
This prevents
exnref.pops from being sinked and separated fromcatch. For example,Here, if we sink
exnref.popto removelocal.set $0andlocal.get $0, it becomes this:This move was possible because
i32.const 3does not have any sideeffects. But this is incorrect because now
exnref.popdoes not followright after
catch.To prevent this, this patch checks this case in
canSinkinSimplifyLocals. When we encountered a similar case in CodeFolding, we
prevented every expression that contains
Popanywhere in it from beingmoved, which was too conservative. This adds
danglingPopproperty inEffectAnalyzer, so that only pops that are not enclosed within acatchcount as 'dangling pops` and we only prevent those pops frombeing moved or sinked.