Skip to content

Add C/JS APIs to copy expressions #2840

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 4 commits into from
May 11, 2020

Conversation

dcodeIO
Copy link
Contributor

@dcodeIO dcodeIO commented May 8, 2020

This API enables use cases where we want to keep the original expression, yet utilize passes like vacuum or precompute to evaluate it without implicitly modifying the original.

C-API: BinaryenExpressionCopy(expr, module)
JS-API: Module#copyExpression(expr)

@dcodeIO
Copy link
Contributor Author

dcodeIO commented May 8, 2020

The test currently fails because I made it to, due to the following problem: When copying an expression, it is straight-forward to make the copied outer expression known to API tracing, but not so straight-forward to do the same for its sub-expressions (see the expressions[0]). My intuition would be that we need a tracing-aware copy algorithm here, but that also means duplicating most of ExpressionManipulator::copy. Any suggestions?

@dcodeIO
Copy link
Contributor Author

dcodeIO commented May 8, 2020

A radical solution might be to remove tracing entirely, since it complicates / limits the C-API with very little benefit imho. Even though it seems nice to have on paper, I actually never used it. Also caught myself several times worrying about the subtle slowdown it introduces since there are like a quadrillion C-API calls in the AssemblyScript compiler.

@kripken
Copy link
Member

kripken commented May 8, 2020

I'm open to removing the tracing API. It was very useful originally with a few early embedders of the C API, but I can't remember the last time it was helpful. Maybe we are stable enough it's not that necessary.

@dcodeIO
Copy link
Contributor Author

dcodeIO commented May 8, 2020

Going to give that a try in a separate PR :)

@dcodeIO
Copy link
Contributor Author

dcodeIO commented May 11, 2020

With API tracing removed this should be good now as well :)

Copy link
Member

@aheejin aheejin left a comment

Choose a reason for hiding this comment

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

Sorry for the delay!

@aheejin aheejin merged commit 655fd6b into WebAssembly:master May 11, 2020
tlively pushed a commit to tlively/binaryen that referenced this pull request May 18, 2020
This API enables use cases where we want to keep the original expression, yet utilize passes like `vacuum` or `precompute` to evaluate it without implicitly modifying the original.

C-API: **BinaryenExpressionCopy**(expr, module)
JS-API: **Module#copyExpression**(expr)
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.

3 participants