Skip to content

Conversation

@dcodeIO
Copy link
Contributor

@dcodeIO dcodeIO commented Dec 9, 2017

These functions will allow code generators to examine some usually side-effect free all expressions, for example to then properly clone Const, GetLocal, GetGlobal and Load expressions where, without that knowledge, a TeeLocal or similar would have to be emitted.

Somewhat related to #1331

(Initially I intended to create a BinaryenExpressionClone API but that didn't play well with tracing when recursively traversing nested expressions, so I figured it might be better to do cloning externally to ensure that proper C-API traces are generated.)

@dcodeIO dcodeIO changed the title Add getters for GetLocal, GetGlobal and Load fields to C/JS Add getters for various specific expression fields to C/JS Dec 9, 2017
BinaryenType BinaryenInt64(void) { return i64; }
BinaryenType BinaryenFloat32(void) { return f32; }
BinaryenType BinaryenFloat64(void) { return f64; }
BinaryenType BinaryenUnreachableType(void) { return unreachable; }
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This makes me sad.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah...

What do you think about changing these to BinaryenTypeInt32, BinaryenTypeUnreachable etc.? We could keep the old ones for backwards compatibility, but I think we can get away with frequent breaking changes, currently.

@dcodeIO
Copy link
Contributor Author

dcodeIO commented Dec 10, 2017

Tests are going to fail now because I haven't touched them yet.

@kripken
Copy link
Member

kripken commented Dec 10, 2017

What tracing issue did you hit with BinaryenExpressionClone? I think we'll want that eventually anyhow.

@kripken
Copy link
Member

kripken commented Dec 10, 2017

IIUC, the API here has getLoad, getBinary etc., and you need to call it with the right one? How about a single method, getInfo (or some better name), which receives a node of any kind and returns the JSON metadata for it (internally it would get the node id and use that)?

@dcodeIO
Copy link
Contributor Author

dcodeIO commented Dec 10, 2017

What tracing issue did you hit with BinaryenExpressionClone? I think we'll want that eventually anyhow.

The issue was that when cloning let's say a set_local(0, i32.const), cloning just the outer expression is fine tracing-wise while cloning the inner one (recursively) would either require calling the C-API method responible for creating it converting from the C++ API to C to C++ again or manually printing the expression[foo] = BinaryenConst(...) trace basically by duplicating the tracing code. Not an unsolvable issue of course.

IIUC, the API here has getLoad, getBinary etc., and you need to call it with the right one? How about a single method, getInfo (or some better name), which receives a node of any kind and returns the JSON metadata for it (internally it would get the node id and use that)?

Yeah, there is getExpression. Not sure about the name though (maybe getExpressionInfo, likewise getFunctionInfo, getConstInfo). Could also make the other ones private, but thought it also doesn't hurt to have them.

@dcodeIO
Copy link
Contributor Author

dcodeIO commented Dec 10, 2017

Hmm, turns out that testing this new API will be somewhat error prone, because infos include pointer values that tend to change when binaryen.js is recompiled :(

@dcodeIO
Copy link
Contributor Author

dcodeIO commented Dec 10, 2017

Not yet implemented: getImportInfo, getExportInfo, getMemoryInfo, getTableInfo. Did I miss something? Might also be cool to have getters (and setters) for custom sections, but that's something for another PR.

@kripken
Copy link
Member

kripken commented Dec 20, 2017

Looks great, thanks!

Anything left to discuss here before merging?

One followup idea I have is to add another field to the JSON infos, that gives their class in JS, so if you get a block's info, you get

{
  what: 'block',
  id: ..
}

Maybe what isn't a great name, but seems like this could make looking at the JSON more convenient? Anyhow, something to think about separate from this PR.

@dcodeIO
Copy link
Contributor Author

dcodeIO commented Dec 20, 2017

One followup idea I have is to add another field to the JSON infos, that gives their class in JS

When I think about it, ideally we'd have TypeScript-like enums first that allow forward and backwards lookups of all these things, like types, expression ids, external kinds, operations etc. Something in the form

Type[Type["none"] = Module['_BinaryenTypeNone']()] = "none";
Type[Type["i32"] = Module['_BinaryenTypeInt32']()] = "i32";
...

ExpressionId[ExpressionId["InvalidId"] = Module['_BinaryenInvalidId']()] = "InvalidId";
ExpressionId[ExpressionId["BlockId"] = Module['_BinaryenBlockId']()] = "BlockId";
...

for each enum-like. With that in place it'd be really easy to look up the numeric ids and string names of everything in both directions. Wdyt?

@kripken
Copy link
Member

kripken commented Dec 20, 2017

Yeah, that makes sense too. I guess it depends on if we want JSON.stringify of these info objects to be self-explanatory, or require the enum lookups. I'm not sure myself.

Anyhow, let's merge this in, and keep thinking about the other stuff.

@kripken kripken merged commit f4b7df0 into WebAssembly:master Dec 20, 2017
@dcodeIO
Copy link
Contributor Author

dcodeIO commented Dec 20, 2017

I guess it depends on if we want JSON.stringify of these info objects to be self-explanatory

We'd then also have to do the same for types I guess? I'd be fine with just the ids, keeping the JSON minimal. Though, we could also make info objects be 'class' instances that provide all sorts of additional utility, in this context for example a getter that returns the string name of an id by implicitly calling the enum utility I proposed above.

@kripken
Copy link
Member

kripken commented Dec 20, 2017

Good point, the type field should be handled the same way.

Not sure what's best for those two fields. Minimal JSON makes sense for efficiency, I suspect, although maybe it doesn't matter much since the strings are going to be internalized anyhow, so they have the same overhead as an integer. And on the other hand, richer JSON, with full string names, seems nicer for people just getting started and for debugging.

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