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

Stack machine + 0xc update #678

Merged
merged 78 commits into from
Sep 7, 2016
Merged

Stack machine + 0xc update #678

merged 78 commits into from
Sep 7, 2016

Conversation

kripken
Copy link
Member

@kripken kripken commented Aug 24, 2016

This brings in all the 0xc and stack machine stuff that are on the spec repo's stack branch. At this point this

  • passes the spec tests except for the new overly-stacky stuff
  • lets asm2wasm pass the emscripten test suite

So it's basically done from my point of view, and if anyone wants to review this now is a good time. However, disabled are

  • those overly-stacky tests, which I think should stay disabled as discussed in Future of Binaryen in a stack machine world? #663
  • wasm-backend and s2wasm tests and functionality are all disabled, as the wasm backend and .s format need to be updated for all these changes. I'm ok to land this before or after that.

@kripken
Copy link
Member Author

kripken commented Aug 24, 2016

(commits starting with TEMP, FIXME, WIP can be ignored and removed before or after landing)

@kripken
Copy link
Member Author

kripken commented Aug 24, 2016

Fuzzing asm2wasm for a while also looks good.

@kripken kripken force-pushed the stack branch 5 times, most recently from e0e091f to 04c67d2 Compare August 29, 2016 17:42
@kripken
Copy link
Member Author

kripken commented Aug 29, 2016

There is a parallel binaryen-stack branch in the emscripten repo now, that is meant to work with this. When we merge this we should merge them together.

@kripken
Copy link
Member Author

kripken commented Aug 29, 2016

The last asm2wasm fixes here allow the emterpreter to run in wasm.

In polyfill testing mode, that means that our program's code is compiled into emterpreter bytecode, which is run in the emterpreter's interpreter, which is compiled from asm.js to wasm, which is run in the polyfill wasm interpreter, which is an interpreter compiled from C++ to asm.js, which is then run in a JS engine. (The meme writes itself.)

@kripken
Copy link
Member Author

kripken commented Aug 30, 2016

Any objections to me cleaning this up a little and landing it?

The main downside is that the wasm backend and s2wasm stuff would all be disabled on master (since it's not updated yet for 0xc/stack). The upside is that this PR is big, and people are filing bugs on master that are already fixed (e.g. emscripten-core/emscripten#4531).

@kripken
Copy link
Member Author

kripken commented Aug 30, 2016

Btw, I ran csmith fuzzing on the asm2wasm path on this branch for 4 days, and found just one minor bug (when running asm2wasm itself compiled to JS, which we do for testing but users would normally run asm2wasm natively, which works ok).

@dschuff
Copy link
Member

dschuff commented Aug 30, 2016

I think I'm almost ready for this. The waterfall has been a bit chaotic lately. Right now the standalone (non-emscripten) tests are running with a frozen version of binaryen to test v8's native wasm mode, so they won't be affected. I'm about to land https://reviews.llvm.org/D24053 which fixes several tests. Currently though the emscripten build on the waterfall is actually broken because of the build warning that @aheejin reported in emscripten-core/emscripten#4501. So maybe we should fix that and get it building again before landing this? Shouldn't be too hard, I'll look into it.

@kripken
Copy link
Member Author

kripken commented Aug 30, 2016

Sounds good. I commented in that issue now with one possible quick fix.

@kripken
Copy link
Member Author

kripken commented Aug 31, 2016

Rebased, cleaned up a little, and re-enabled spec tests on latest upstream stack branch. This is good to land whenever you are ready.

@kripken
Copy link
Member Author

kripken commented Sep 1, 2016

Spec repo now has a PR with the new import/export stuff with a concrete syntax, which differs from the temporary one I guessed here. Added a commit for imports now.

@kripken
Copy link
Member Author

kripken commented Sep 2, 2016

@dschuff, ping - are you ok with this landing?

Meanwhile I did some more parsing work here for upcoming spec repo changes.

@dschuff
Copy link
Member

dschuff commented Sep 2, 2016

I think so. Is it still the case that s2wasm/wasm backend is completely disabled? I guess what we have to do is just update the wasm building code in s2wasm, so that it builds the new IR?

@kripken
Copy link
Member Author

kripken commented Sep 2, 2016

Yes, in this PR that's disabled. There are the handwritten and autogenerated tests etc. that need to be fixed up, unless you've already done that?

If not, what do you think about landing this first?

@kripken
Copy link
Member Author

kripken commented Sep 3, 2016

Anyone know why appveyor keeps failing? There isn't a build to check an error on, weird.

@kripken
Copy link
Member Author

kripken commented Sep 6, 2016

@dschuff: ping - are you ok to land this? (needs a merge fix which I'll fix, and later on master we can figure out the .s/wasm-backend tests)

@dschuff
Copy link
Member

dschuff commented Sep 7, 2016

Yeah, go ahead and land it. Thanks for the heads-up.

On Tue, Sep 6, 2016, 4:45 PM Alon Zakai notifications@github.com wrote:

@dschuff https://github.com/dschuff: ping - are you ok to land this?
(needs a merge fix which I'll fix, and later on master we can figure out
the .s/wasm-backend tests)


You are receiving this because you were mentioned.

Reply to this email directly, view it on GitHub
#678 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/ABEiKH0y2PC2H7achrKPG-j8vJ2Fw9xuks5qnfsOgaJpZM4Jrn_s
.

… input is not yet valid

then after finalizeCalls, we must autodrop again to drop things that finalizeCalls changed
apply memory segments only if there isn't a memory initializer (which we need for asmjs and asm2wasm modes)

use wasm-opt to check recreated wasts for validity, as wasm-shell would try to execute them

add testing for combined modes like asmjs,interpret-binary
@kripken
Copy link
Member Author

kripken commented Sep 7, 2016

Ok, rebased and tests passed, merging.

@kripken kripken merged commit 135a20c into master Sep 7, 2016
@kripken kripken deleted the stack branch September 7, 2016 17:55
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