Skip to content

Optimize block tails where a dropped br_if's value is redundant #7506

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

Merged
merged 25 commits into from
May 2, 2025

Conversation

xuruiyang2002
Copy link
Contributor

If a block ends with a br_if followed by a value that is the same as the br_if's value (and has no side effects), the value of br_if and br_if itself are redundant and can be removed. For example:

(block $block (result i32)
 ..
 (drop
  (br_if $block
   (value)
   (condition)
  )
 )
 (value)
)

=>

(block $block (result i32)
 ..
 (drop
  (condition)
 )
 (value)
)

Fixes: #7489

@xuruiyang2002
Copy link
Contributor Author

Thanks! Learned a lot from your comments and good coding taste. I've updated as you suggested.

xuruiyang2002 and others added 7 commits April 25, 2025 09:23
Avoid redundant constant

Co-authored-by: Alon Zakai <alonzakai@gmail.com>
Co-authored-by: Alon Zakai <alonzakai@gmail.com>
Co-authored-by: Alon Zakai <alonzakai@gmail.com>
Co-authored-by: Alon Zakai <alonzakai@gmail.com>
Co-authored-by: Alon Zakai <alonzakai@gmail.com>
Co-authored-by: Alon Zakai <alonzakai@gmail.com>
Copy link
Member

@kripken kripken left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good aside from two final comments. Also please fuzz this for a while.

xuruiyang2002 and others added 3 commits April 30, 2025 11:47
@xuruiyang2002
Copy link
Contributor Author

xuruiyang2002 commented Apr 30, 2025

(Fixed)

Bad news, the fuzzer told me that this PR aborts when dealing with the following wat code:

(module
 (rec
  (type $0 (struct))
  (type $1 (func (param funcref) (result (ref $0))))
  (type $2 (func))
 )
 (func $0 (type $2)
  (nop)
 )
 (func $1 (type $1) (param $0 funcref) (result (ref $0))
  (block $block (result (ref $0))
   (drop
    (br_on_cast $block (ref $0) (ref $0)
     (struct.new_default $0)
    )
   )
   (struct.new_default $0)
  )
 )
 (func $2 (type $2)
  (drop
   (call $1
    (ref.func $0)
   )
  )
 )
)

Above code saved as w.wat

Compile option: bin/wasm-opt w.wat --code-folding --monomorphize --pass-arg=monomorphize-min-benefit@95 --optimize-level=0 --shrink-level=0 -fimfs=99999999 --closed-world --dce --enable-threads --enable-mutable-globals --enable-nontrapping-float-to-int --enable-simd --enable-bulk-memory --enable-sign-ext --enable-exception-handling --enable-tail-call --enable-reference-types --enable-gc --enable-memory64 --enable-extended-const --enable-multimemory --enable-stack-switching --enable-bulk-memory-opt --enable-call-indirect-overlong --closed-world -S -o -

wasm-opt complains at src/wasm-traversal.h:286: :

void wasm::Walker<SubType, VisitorType>::pushTask(TaskFunc, wasm::Expression**) [with SubType = wasm::RemoveUnusedNames; VisitorType = wasm::UnifiedExpressionVisitor<wasm::RemoveUnusedNames>; TaskFunc = void (*)(wasm::RemoveUnusedNames*, wasm::Expression**)]: Assertion `*currp' failed.
Aborted (core dumped)

Obviously, the optimization meets all the condition and performs the update, however, the br->condition is null, and drop cannot ref to a null ptr.

Fixed.

Updated: I've fuzzing for one hour, all is well.

@kripken
Copy link
Member

kripken commented Apr 30, 2025

Good, nice, this is exactly what the fuzzer is helpful with!

@xuruiyang2002
Copy link
Contributor Author

After updating all you suggested, I've also fuzzing for about 15 minutes; and all is well.

@kripken kripken enabled auto-merge (squash) May 2, 2025 21:39
@kripken kripken merged commit 0934d6c into WebAssembly:main May 2, 2025
15 checks passed
@xuruiyang2002 xuruiyang2002 deleted the br_if_value_redundant branch May 3, 2025 06:49
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.

[Unhoisted] [Binary] [Might not fallthrough] load & store order affects the constant propagation in binaryOp
2 participants