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.

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

Potential optimization opportunity: Tablify sequences of ifs #2724

Closed
dcodeIO opened this issue Apr 6, 2020 · 4 comments
Closed

Potential optimization opportunity: Tablify sequences of ifs #2724

dcodeIO opened this issue Apr 6, 2020 · 4 comments

Comments

@dcodeIO
Copy link
Contributor

dcodeIO commented Apr 6, 2020

So far we have been using sequences of br_ifs to hint to Binaryen that these might be suitable candidates for a br_table if conditions are dense constants potentially. While this works well, there are occasions where it is easier / more idiomatic to emit sequences of normal ifs, either manually or using the Reloopers addBranch, but these don't benefit from RemoveUnusedBrs's tablify. Typical code is:

(local.set $0 (...))
(if
 (i32.or
  (i32.or
   (i32.eq
    (local.get $0)
    (i32.const 1)
   )
   (i32.eq
    (local.get $0)
    (i32.const 2)
   )
  )
  (i32.eq
   (local.get $0)
   (i32.const 3)
  )
 )
 (if
  ...
  (call ...)
 )
 (call ...) ;; or unreachable
)

or

(local.set $0 (...))
(if
 (i32.or
  (i32.or
   (i32.eq
    (local.get $0)
    (i32.const 1)
   )
   (i32.eq
    (local.get $0)
    (i32.const 2)
   )
  )
  (i32.eq
   (local.get $0)
   (i32.const 3)
  )
 )
 (return
  (call ...)
 )
)
(if
 ...
 (return
  (call ...)
 )
)
(call ...) ;; or unreachable

where at some point converting to a br_table seems to become more efficient.

@kripken
Copy link
Member

kripken commented Apr 7, 2020

This type of pattern is pretty hard to match in general. How about doing the opposite, creating a br_table in your code, and let the optimizer lower that to ifs if that makes sense? I think that would be much more practical.

@dcodeIO
Copy link
Contributor Author

dcodeIO commented Apr 7, 2020

I see, hmm. The problem with emitting a br_table is that it can potentially be huge, making the code hard to deal with in our tests, or for anyone looking at untouched output for their own reasons.

But, reading your comment, I'm wondering if there is already something in place (that I missed) to lower such a table right away, upon the Relooper's renderAndDispose? If not, that'd essentially solve the issue as well for me, since one could simply build a table and rely on the Relooper to do the right thing right away, based on optimize levels set on render.

@dcodeIO
Copy link
Contributor Author

dcodeIO commented Apr 8, 2020

Created a little helper on our end now that builds a br_ifified table, with an API very similar to the Relooper's. Does the job, so the suggestions here aren't particularly important anymore and more a nice to have :)

@tlively
Copy link
Member

tlively commented Jan 13, 2025

Closing because the requested feature is no longer necessary.

@tlively tlively closed this as not planned Won't fix, can't repro, duplicate, stale Jan 13, 2025
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

No branches or pull requests

3 participants