Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Optimization passes removing block results required by catch clauses #6724

Closed
frank-emrich opened this issue Jul 10, 2024 · 3 comments
Closed

Optimization passes removing block results required by catch clauses #6724

frank-emrich opened this issue Jul 10, 2024 · 3 comments

Comments

@frank-emrich
Copy link
Contributor

I'm seeing a couple of cases where optimization passes invalidate modules using EH instructions by removing seemingly unnecessary block results.

Consider the following module:

(module
 (tag $t (param i32))

 (func $f (export "f")
  (block $handler (result i32)
   (try_table (catch $t $handler)
    (throw $t (i32.const 123))
   )
   (unreachable)
  )
  (drop)
 )
)

Running, say, the vacuum pass on this removes the i32 result of handler, which then invalidates the module:

$ bin/wasm-opt --vacuum -all test.wat        
[wasm-validator error in function f] break type must be a subtype of the target block type, on 
(block $handler
 (try_table (catch $t $handler)
  (throw $t
   (i32.const 123)
  )
 )
)
Fatal: error after opts

I haven't dug deeply into the root cause, but I'm seeing validation errors when running any of the following passes invidiually on the module above:

  • gufa
  • gufa-cast-all
  • local-subtyping
  • string-lowering
  • string-lowering-magic-imports
  • type-ssa
  • unsubtyping
  • vacuum

I'm not sure what the status of the Binaryen implementation of the updated EH proposal (using try_table) is, so please let me know if this is just me using things that aren't supposed to be working, yet.

I'm interested in these problems regarding EH code because they also break typed continuations code in a completely analogous way: A resume instruction's tag clauses require the result types of the handler blocks it mentions to remain unchanged. I'm thus hoping that I will be able to re-use the fixes for the EH issue described here.
@kripken
Copy link
Member

kripken commented Jul 10, 2024

I think there are TODOs for optimizing try_table, is that right @aheejin ? E.g. on that testcase I see

$ bin/wasm-opt a.wat -all -O1
unimplemented DCE control flow structure
UNREACHABLE executed at src/passes/DeadCodeElimination.cpp:189!
Aborted

In a build without assertions I guess it might be less clear, sorry about that @frank-emrich

@aheejin
Copy link
Member

aheejin commented Jul 10, 2024

Yes, the new EH instructions are currently only supported for being emitted at the end of the pipeline via --emit-exnref and re-feeding that output into the optimization pipeline is not supported yet.

@frank-emrich
Copy link
Contributor Author

Ah, no problem! I just started working on making the optimizer work on typed continuations instructions (resume and friends). When I noticed that the optimizer was breaking certain resume instructions, I just wondered if it would do the same with try_table, realized that it did, and opened this issue. I somehow thought that optimization for EH instructions was generally working and that I could re-use any fix for try_table and apply it to resume, too 😄

Just for reference, I've now identified what was causing the particular problem I was seeing, both for resume and try_table:
Both ReFinalize::visitResume and ReFinalize::visitTryTable need to call updateBreakValueType with their respective sentTypes. Without these calls, the refinalizer may not notice that some resume or try_table need certain block results to stay.

That being said, I think it's sensible to just close this issue. I will update ReFinalize::visitResume as part of my upcoming PRs to make the optimizer work on typed continuations instructions. When doing so I will mention that whatever I'm doing for visitResume should probably be done identically for visitTryTable (or I'll just do it myself while I'm at it).

@tanishiking tanishiking mentioned this issue Sep 10, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@kripken @aheejin @frank-emrich