Skip to content

Don't generate redundant unreachables in Flatten #2583

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

Closed
wants to merge 2 commits into from

Conversation

aheejin
Copy link
Member

@aheejin aheejin commented Jan 11, 2020

This is to remove a code pattern that generates an additional
unreachable after an unreachable expression. This happens when the
unreachable expression is a direct child of a block/loop/try.
Block/loop/try preserves its childrens' preludes within it, which means
childrens' preludes are not gonna escape the structure. So this kind of
code pattern is often generated by Flatten:

(block
  (some unreachable expression)
  (unreachable) ;; unnecessary
)

This PR removes the unnecessary unreachables generated.

On the other hand, if does not satisfy that property because if's
condition's prelude can escape if.

This is not for optimization (Flatten is not for optimization per se;
it's more of an enabler of other optimization passes, so we don't need
to optimize the code generated by Flatten because we have other passes
after this); This is done in order to make for #2567 to work. We are
planning to disable Flatten for exceptions now, but that's because of
br_on_exn; try handling part (#2567) can land now. Current Flatten
generates unnecessary blocks because it generates unnecessary
unreachables and wrap the original unreachable expression and the new
unreachable instruction within a block, ending up generating an
extra blocks within catch, which is a problem, because this can
violate the invariant that exnref.pop should follow after catch.

@aheejin aheejin requested a review from kripken January 11, 2020 00:45
@aheejin
Copy link
Member Author

aheejin commented Jan 11, 2020

Turning on 'Ignore whitespace changes' makes reviewing test cases easier. Also I increased the log length limit to 500 in fuzz_opt.py, which I think is better for debugging in case something bad happens.

This is to remove a code pattern that generates an additional
`unreachable` after an unreachable expression. This happens when the
unreachable expression is a direct child of a block/loop/try.
Block/loop/try preserves its childrens' preludes within it, which means
childrens' preludes are not gonna escape the structure. So this kind of
code pattern is often generated by Flatten:
```wast
(block
  (some unreachable expression)
  (unreachable) ;; unnecessary
)
```
This PR removes the unnecessary `unreachable`s generated.

On the other hand, `if` does not satisfy that property because `if`'s
condition's prelude can escape `if`.

This is not for optimization (Flatten is not for optimization per se;
it's more of an enabler of other optimization passes, so we don't need
to optimize the code generated by Flatten because we have other passes
after this); This is done in order to make for WebAssembly#2567 to work. We are
planning to disable Flatten for exceptions now, but that's because of
`br_on_exn`; try handling part (WebAssembly#2567) can land now. Current Flatten
generates unnecessary `block`s because it generates unnecessary
`unreachable`s and wrap the original unreachable expression and the new
`unreachable` instruction within a `block`, ending up generating an
extra `block`s within `catch`, which is a problem, because this can
violate the invariant that `exnref.pop` should follow after `catch`.
@aheejin aheejin force-pushed the flatten_remove_unreachable branch from 3350039 to dac1961 Compare January 11, 2020 00:50
@dcodeIO
Copy link
Contributor

dcodeIO commented Jan 11, 2020

Nice, this will also make inspecting intermediate pass output more convenient. FWIW: Iirc I also had a hard time reading output due to flatten flattening already flat code (like introducing another local for a local.set that is already flat). Actually tried looking into that once with little success. Though, in the process I found that a good way to highlight this is to run flatten multiple times while observing exponential code growth. Even if not realistic, it shows where the first run is already doing unnecessary work that subsequent passes (and LocalGraph) will have to deal with. Perhaps another optimization opportunity :)

@aheejin
Copy link
Member Author

aheejin commented Jan 13, 2020

This is my second PR to remove unnecessarily generated instructions in Flatten. First work is at #2524.

#2524 removes unnecessary nops and this removes unnecessary unreachables. I also tried to remove seemingly redundant and repetitive local.sets and local.gets before, but it didn't work well - I discovered at least some of the cases needed an extra local.get/local.set pair even though they looked seemingly redundant at first glance. The cases I discovered were related to local.tee and br_if, but there possibly can be more. At that point I dropped it, because, while it might be possible that redundant local.sets/local.gets can be removed in some cases, adding more code to Flatten on those specific conditions will complicate the pass, and I didn't think it would have much benefit that can compensate the code complexity, because we usually run other optimization passes (such as Vacuum, SimplifyLocals, ..) anyway to remove all those redundant locals.

But anyway, as I said, this removal of unreachable was necessary to make #2567 work, so..

@aheejin
Copy link
Member Author

aheejin commented Jan 13, 2020

I'll hold this for now, because the fuzzer discovered a new bug. I'll post the bug here for future reference, even though I don't think I'll work on this soon.

(module
 (func $test (result i32)
  (loop $label$1 (result i32)
   (block $label$2
    (br_if $label$1
     (i32.eqz
      (i32.const 0)
     )
    )
    (nop)
    (block $label$3
     (loop $label$4
      (unreachable)
     )
    )
   )
  )
 )
)

Failing command:

wasm-opt --flatten --rereloop t.wast

Base automatically changed from master to main January 19, 2021 21:59
@aheejin
Copy link
Member Author

aheejin commented Nov 22, 2021

Given that #2567 is not dependent on this anymore, I think we can close this old PR.

@aheejin aheejin closed this Nov 22, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants