Skip to content

Running passes on a single function in binaryen-c/.js - #1295

Merged
kripken merged 5 commits into
WebAssembly:masterfrom
dcodeIO:js-runpasses-on-function
Nov 21, 2017
Merged

kripken merged 5 commits into
WebAssembly:masterfrom
dcodeIO:js-runpasses-on-function

Conversation

@dcodeIO

@dcodeIO dcodeIO commented Nov 16, 2017 •

Copy link
Copy Markdown
Contributor

As an alternative to #1294, instead of adding a more specific API, this PR adds an additional optional argument to BinaryenModuleRunPasses (breaks the C-API, but RunPasses is relatively new and this can be transparently handled in JS) to run passes on a single function only.

@kripken

kripken commented Nov 17, 2017

Copy link
Copy Markdown
Member

How about adding a new function for this, BinaryenFunctionRunPasses? (I don't care about breaking backwards compatibility on this, but it seems cleaner to me.)

Btw, is this enough for your use case of optimizing just an expression? We can add that too, but it requires work inside binaryen to add it first. I can do that if you want.

@kripken

kripken commented Nov 17, 2017

Copy link
Copy Markdown
Member

Also would be nice to add BinaryenFunctionOptimize to parallel BinaryenModuleOptimize (i.e., run the default opt passes on a function). Can leave it to later too.

@dcodeIO

dcodeIO commented Nov 17, 2017 •

Copy link
Copy Markdown
Contributor Author

Thought to add an additional parameter because the function requires the module as a parameter anyhow. In terms of a nice API, BinaryenFunctionRunPasses would ideally take a funktion reference as its first parameter (at least that's the naming scheme I have in mind, alternative here is BinaryenModuleRunPassesOnFunction).

Regarding my use case: Somewhat. I'd then use the initializer expression I'd like to precompute as the body of a new function, optimize that, obtain a reference to the updated body (here: BinaryenFunctionGetBody, still todo), remove the function again, evaluate the expression's kind expecting a const (here: BinaryenExpressionGetId) and finally replace the initializer of the respective global.

On BinaryenFunctionOptimize: Sure, initially added that as well (as BinaryenModuleOptimizeFunction), but then settled for the general solution because of previous comments. Not a big deal to add it as well, though.

@kripken

kripken commented Nov 17, 2017

Copy link
Copy Markdown
Member

It's true the implementation is almost the same since the function version needs the module anyhow, good point. Maybe internally they can use a shared helper function. For the external API, that's a good question about those options. I guess it depends on whether we expect to have more BinaryenFunction* methods. I suspect yes (get params, change result, etc.). So I am leaning to BinaryenFunctionRunPasses, taking the module as the final param (like RelooperRenderAndDispose).

About the second paragraph, I think that's a general enough thing that I'd like a binaryen utility for it. It should also handle locals etc. properly, so it's not as simple as your use case, but feasible. If that makes sense I can do that later today (unless you want to).

@dcodeIO

dcodeIO commented Nov 17, 2017

Copy link
Copy Markdown
Contributor Author

So I am leaning to BinaryenFunctionRunPasses, taking the module as the final param (like RelooperRenderAndDispose).

Sounds good.

About the second paragraph, I think that's a general enough thing that I'd like a binaryen utility for it. It should also handle locals etc. properly, so it's not as simple as your use case, but feasible. If that makes sense I can do that later today (unless you want to).

That'd be great, I'm not too familiar with the internals yet.

@dcodeIO

dcodeIO commented Nov 18, 2017 •

Copy link
Copy Markdown
Contributor Author

One thing about splitting up module and function entrypoints (BinaryenModuleRunPasses+ BinaryenFunctionRunPasses) I see is that, while it fits well to the C-API, the JS-API doesn't benefit from the split because it can have methods on the module instance (with the module parameter substituted). Module#runPasses, for example, is a member of module instances already. When removing the optional functionName parameter from it, there'd have to be a Module#runPassesOnFunction which we tried to avoid in the C-API - or - a static Module.functionRunPasses taking the module as its second argument.

I see two ways to deal with that: 1) Simply accept that the JS-API differs and keep functionName as an optional parameter just there or 2) use that additional parameter in C (BinaryenModuleRunPasses) as well to make it uniform.

@kripken

kripken commented Nov 21, 2017

Copy link
Copy Markdown
Member

About the general optimize-an-expression thing, as I was writing it I realized it can't be efficient, at least if it tries to handle the general case (you need to do a bunch of copying and scanning, stuff you don't need when you optimize a whole function). So I'm starting to think it's not worth it. Do you have enough for your use case without it?

About the C and JS APIs, I think it's best to let each make sense on its own, they don't need to be a perfect parallel. As you said in the past, we may have other APIs built on top of the C API eventually anyhow.

@dcodeIO

dcodeIO commented Nov 21, 2017

Copy link
Copy Markdown
Contributor Author

The latest commit adds the following JS-API as well:

  • Module#optimizeFunction accepting both a name or a function reference
  • Module#runPassesOnFunction accepting both a name or a function reference

I also added the minimum required utility functions to make a test case from my specific use case (see binaryen.js/functions.js):

  • BinaryenGetFunction / Module#getFunction
  • BinaryenRemoveFunction / Module#removeFunction
  • BinaryenFunctionGetBody / getFunctionBody

@dcodeIO dcodeIO changed the title Optionally run passes on a single function only in binaryen-c/.js Running passes on a single function in binaryen-c/.js Nov 21, 2017
return Module['_BinaryenModuleOptimize'](module);
};
this['optimizeFunction'] = function(func) {
if (typeof func === "string") func = this['getFunction'](func);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

this overloading could also work in removeFunction? Could be nice there too as a followup maybe.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Not yet because, unlike BinaryenFunctionOptimize, BinaryenRemoveFunction expects a name, not a reference. What I'm trying to establish, though, is using names in BinaryenAddSomething-style functions, and Something-references in BinaryenSomethingAdd-style ones. Here, that'd be BinaryenFunctionRemove (note: doesn't fit well to the C++ API because wasm->removeFunction expects a name).

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Hmm, yeah. But we could eventually do the reverse, maybe: the C API expects a name, so if JS receives a name, pass that through. And if you get a number (a pointer), you can call FunctionGetName (which returns the name, parallel to GetBody), and use that.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

When doing the reverse, that is optionally resolving a reference to its name everywhere, we'll end up decoding and encoding even more strings :(

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Yeah, it wouldn't be optimal.

@kripken
kripken merged commit eedcc29 into WebAssembly:master Nov 21, 2017
@tlively tlively mentioned this pull request Nov 6, 2023
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