Skip to content

Candidate for 0xc AngryBots #14

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 8 commits into from
Sep 14, 2016
Merged

Candidate for 0xc AngryBots #14

merged 8 commits into from
Sep 14, 2016

Conversation

sunfishcode
Copy link
Member

Update to the remaining anticipated 0xc features.

Additional Changes:

  • Block signatures.
  • Section opcodes.
  • Minimally update Release/AngryBots.js to work with the new JS API.
  • Include a binary-encoded wasm file in 0xc format.

I made up a syntax for block signatures in the flat text format, taking
care to allow for adding arguments and multiple results in the future.
For example:

      block $label28 () : (i32)

Is a block no arguments and one return type which is i32.

However, I'm not attached to this particular syntax, and it can easily
be converted to something else if desired.

This adds 0xc changes, except block signatures.

Changes:

 - converted loops to not use their exit label
 - updated indirect_call operand order to put the callee last
 - inserted a gratuitous dependence on stack-machine rules (see the very
   first set_local)
 - inserted a gratuitous use of branch-to-function scope
 - optimized away unnecessary `blocks`
 - new "flat" text format
Changes:
 - Convert to block signatures.
 - Convert to section opcodes.
 - Minimally update Release/AngryBots.js to work with the new JS API.
 - Include a binary-encoded wasm file in 0xc format.

I made up a syntax for block signatures in the flat text format, taking
care to allow for adding arguments and multiple results in the future.
For example:

```
      block $label28 () : (i32)
```

Is a `block` no arguments and one return type which is i32.

However, I'm not attached to this particular syntax, and it can easily
be converted to something else if desired.
Use the block signature syntax implemented here:

WebAssembly/spec#336
@sunfishcode
Copy link
Member Author

The .wast file is now updated to use the block signature syntax of WebAssembly/spec#336.

@s3ththompson
Copy link
Member

This won't run until we add polyfill support, right?

@sunfishcode
Copy link
Member Author

The index.html file uses Wasm.experimentalVersion to determine the filename to load. I've now renamed the wasm file to match the name it expects, so this should run in browsers that support the format.

@s3ththompson
Copy link
Member

Makes sense. Let me know when you're finished updating the binary and I'll lgtm

@sunfishcode
Copy link
Member Author

I've finished all the updates I'm aware of at this time.

The binary runs in my development tree of Firefox, which has all the anticipated 0xc binary format changes I'm currently aware of implemented.

@s3ththompson
Copy link
Member

Got it. Weren't we going to switch over to the WebAssembly object, though?

Add a path to use the new JS API with Module and Instance, and use it
instead of the old Wasm.instantiatemodule when wasm.experimentalVersion
is not less than 0xc.
@sunfishcode
Copy link
Member Author

I've now updated the Angrybots.js script to switch to the new JS API, keeping the old Wasm.instantiateModule path in place when Wasm.experimentalVersion is less than 0xc.

var instance;
instance = Wasm.instantiateModule(binary, info).exports;
mergeMemory(instance.memory.buffer);
if (Wasm.experimentalVersion < 0xc) {
Copy link
Member

Choose a reason for hiding this comment

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

perhaps if (Wasm.experimentalVersion < 0xc || typeof WebAssembly == "undefined") would be more robust (in case some engines haven't yet switched over)?

@s3ththompson
Copy link
Member

LGTM modulo minor nit

@sunfishcode
Copy link
Member Author

Nit fixed.

@titzer
Copy link

titzer commented Sep 14, 2016

I think we might have jumped the gun on landing this, as we haven't yet tried out these bits in V8.

@s3ththompson
Copy link
Member

@titzer Sorry, I didn't check yet because we haven't revved Wasm.experimentalVersion in Canary and I figured we can always update this binary if we find a problem.

@lukewagner
Copy link
Member

Well, almost: we were about to land our 0xc patch which, after landing, would make updating this binary more of a hassle.

@titzer Any ETA on V8 confirmation of the binary?

@jfbastien
Copy link
Member

Right, I think this was merged too quickly: we should be more disciplined about releasing binaries which we may just break in the future. No big deal, but let's not to that again.

@titzer
Copy link

titzer commented Sep 14, 2016

We should probably discuss how long we want browsers to support 0xC (and thus only demo bits) until the toolchain and everything else is also generating 0xC. I think we need a day or two to verify this binary and the other spec tests.

@s3ththompson
Copy link
Member

Sorry folks. Thought this was less of an issue given that the codepath won't be hit until we all rev Wasm.experimentalVersion, but I'll revert now for further discussion. Lesson learned.

@s3ththompson
Copy link
Member

Looks like I need an LGTM on the new revert PR

@lukewagner
Copy link
Member

@titzer That's a good point. Having the binary verified will be a good first step, though, in demonstrating we're all on the same page.

@s3ththompson
Copy link
Member

Sorry for the churn. @sunfishcode would you mind rebasing your changes against the rebase and re-opening?

sunfishcode referenced this pull request Sep 14, 2016
This is the same change as https://github.com/WebAssembly/webassembly.github.io/pull/14.

Note that it includes the changes described in WebAssembly/design#740
however it doesn't have any user-defined or names sections, so it's
unaffected by the details of how those are handled.
@sunfishcode sunfishcode deleted the testing-0xc-candidate branch September 14, 2016 19:10
@sunfishcode
Copy link
Member Author

Rebased and reopened in #16.

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.

5 participants