Get wasm2asm building again by tlively · Pull Request #1107 · WebAssembly/binaryen

tlively · 2017-07-21T20:25:41Z

Updates CMakeLists.txt to have wasm2asm built by default, updates
wasm2asm.h to account for recent interface changes, and restores
JSPrinter functionality.

Updates CMakeLists.txt to have wasm2asm built by default, updates wasm2asm.h to account for recent interface changes, and restores JSPrinter functionality.

tlively · 2017-07-21T20:27:51Z

This the beginning of my work on wasm2asm. Getting it fully functional will allow me to use it in the Rust compiler to support asm.js and eventually remove Rust's dependency on Fastcomp.

cc @eholk @jgravelle-google @kripken

…n-local

kripken · 2017-07-22T00:01:03Z

Glad to see interest in working on this :)

We should figure out a plan for how this will work - perhaps you've already thought through these points, but these are the issues that stalled the initial work on this:

Handling control flow, wasm allows e.g. a loop inside an if condition, which asm.js does not.
i64 support.
Emitting reasonably efficient asm.js code. The first two issues can mostly be fixed up with adding temporary locals, but then we need to optimize those out somehow.

The good news is that I think (1) can be handled by running the flatten-control-flow pass first; then what is left is much simpler and should map 1:1 to asm.js. And (3) is much less of an issue since the binaryen optimizer is now good enough to optimize away temp locals like that, so all optimization could be done in binaryen, without depending on running an asm.js optimizer.

Also the tradeoffs need to be thought out. Sounds like your focus is on offline compilation, while I think it's also useful to have client-side compilation (e.g., a polyfill). That could affect the design here.

Thoughts?

tlively · 2017-07-24T16:33:58Z

@kripken thanks for bringing up those issues. My hope is that the control flow flattening will work, but if it doesn't then I'm prepared to do work to find a transformation. I will probably try to emit and use library code for i64 support initially, just to get something working, but it would probably be good to emit smarter code later.

My priority is definitely offline compilation, but I agree that client-side seems useful as well. What kind of tradeoffs do you think would be important there?

kripken · 2017-07-24T16:57:09Z

The main tradeoff there is compilation speed, I think. But maybe it's not an issue any more, in the past we would have needed to run the slower asm.js optimizer on the asm.js, but now it's probably enough to just run the fast binaryen optimizer before doing the conversion to asm.js. Could be fine on the client.

jgravelle-google · 2017-07-24T16:44:14Z

+    emit(node->getCString());
+  }
+
+    static char* numToString(double d, bool finalize=true) {


This line seems overindented

jgravelle-google · 2017-07-24T16:45:05Z

  }
  // figure out the table size
-  tableSize = wasm->table.names.size();
+  // tableSize = wasm->table.names.size();


Remove commented-out code

jgravelle-google · 2017-07-24T16:45:47Z

-      table.resize(tableSize);
-      for (size_t j = 0; j < tableSize; j++) {
-        table[j] = fromName(name);
+  //  for (size_t i = 0; i < wasm->table.names.size(); i++) {


jgravelle-google · 2017-07-24T16:49:10Z

        }
-        default: abort();
+        default:
+          std::cerr << curr << '\n';


Yay error messages.

Might be worth adding more context though? "Unexpected type while visiting const" or something?

jgravelle-google · 2017-07-24T16:53:36Z

+  // Utils
+
+  void ensure(int safety=100) {
+    if (size < used + safety) {


Can reduce a level of nesting here by changing this to early-return style:

if (size >= used + safety) { return; }

jgravelle-google · 2017-07-24T16:55:56Z

+      return;
+    }
+    if (node->isAssign()) {
+      printAssign(node);


No return after printAssign?

This code had previously been deleted, so I just brought it back exactly how it was. I don't see why there wouldn't be a return here, but I don't want to mess with it before I have tests running.

jgravelle-google · 2017-07-24T16:57:23Z

+        break;
+      }
+      default: {
+        printf("cannot yet print %s\n", type.str);


Should this print to stderr?

jgravelle-google

Looking at the test dir briefly I found this:
https://github.com/WebAssembly/binaryen/blob/master/test/address.2asm.js
Which was added in this commit: 299ab38#diff-391e3722bdf96d68c289a54f04d54d97
There's also this commit that removed the wasm2asm tests from check.py: 0783d60#diff-80ee07c6b10902b1bae15660a736de65

So, this PR should probably turn those tests on too? Or at least bring them back if 0:'d, and ran locally

jgravelle-google · 2017-07-24T21:39:12Z

  }
-  // if (theVar[1]->size() == 0) { XXX ???
+  // if (theVar[1]->size() == 0) {
+  //   XXX What should splice do???


Ah, good reword of the comment here to be more explicit about what's "???"

Yeah this seems like the kind of thing we should know? But also seems like the kind of thing that should already be broken. Which ties in nicely to the whole concept of "having tests."

Which is to say I'm ok with leaving this as-is until we have a failing test to motivate what it should be. I also think we should get some tests for pretty much asap.

kripken · 2017-07-24T21:49:15Z

Test-wise it would be good to (eventually, not necessarily in this PR, depending on when it lands) have some reference tests (i.e., compare the asm.js output to a correct output) and functional tests. For functional tests,

When we run the wasm spec tests, we should see that we can wasm2asm each one and run it as asm.js with the same output.
When we run full compiled programs compiled with emcc, we could compile the wasm part into asm.js and see that the program runs as expected.

It would be good to have both, although the main use case discussed here would be the second of those, so the first might be left for later.

jgravelle-google · 2017-07-27T16:28:51Z

 run_command(ASM2WASM + [asmjs, '-o', 'b.wast', '-S'])
 assert open('b.wast', 'rb').read()[0] != '\0', 'we emit text with -S'

+''' wasm2asm tests disabled for now. Use scripts/test/wasm2asm.py to test


Better idea: have check.py import scripts.test.wasm2asm as wasm2asm, wrap the check code in a function, check_wasm2asm, and call it from here. (Then comment out the call)

tlively · 2017-07-27T19:46:47Z

I was failing CI earlier because of unused imports in wasm2asm.py. Does that lint only run on the files under scripts?

Edit: It looks like it does only run for scripts

jgravelle-google · 2017-07-27T20:03:35Z

Oh, I didn't think about that as being a lint failure but no yeah that makes sense. Probably fine as-is then.

tlively · 2017-07-27T22:59:46Z

The difference in parentheses and semicolons between the current code and the previous working version of wasm2asm is due to the removal of STAT nodes in f6c0bc2. @kripken do you know if there are any situations in which missing these semicolons would be semantically incorrect? If so, I should bring back STAT nodes.

jgravelle-google · 2017-07-27T23:09:45Z

    raise Exception(('run_command failed', err))
-  if expected_err is not None and err != expected_err:
+  err_correct = expected_err is None or \
+    (expected_err in err if err_contains else expected_err == err)


Why not indent this line two extra spaces, instead of disabling that warning project-wide?

kripken · 2017-07-28T20:27:24Z

Hmm, STAT nodes only matter for printing of JS, they say where to emit a semicolon, and the asm.js subset is simple enough that I think we'd either be ok without semicolons, or would know trivially where to add them. In other words, we might need to change the asm.js AST => asm.js text code a little, but I'm hoping not much, probably just something like add a semicolon

after each statement in a block
after the if body (and else if there is one)
after the for/do/while loop body

tlively · 2017-07-28T21:12:21Z

Cool, I know semicolons are emitted after ifs and loops, so I just added a special case for statements in blocks. This PR should be good to go now.

kripken

Overall this looks ok, aside from one issue in the tests - is that a temporary limitation, no longer emitting x = x | 0; etc.? And are the tests purposefully disabled?

I didn't read the logic changes too carefully, it'll be easier to just see test outputs when they are available.

kripken · 2017-07-31T20:21:23Z


 s2wasm.test_s2wasm()
 s2wasm.test_linker()
+# wasm2asm.test_wasm2asm()


intentionally not enabled?

Yes, since there aren't many tests right now anyway. I was planning to enable the tests in a future PR that would finish 32 bit support and add more tests.

Sounds good.

kripken · 2017-07-31T20:21:53Z

  if proc.returncode != expected_status:
    raise Exception(('run_command failed', err))
-  if expected_err is not None and err != expected_err:
+  err_correct = expected_err is None or \


why do we need the new contained-but-not-equal mode? is equality too much for some test?

Yes, the MOZJS case in test_wasm2asm() cannot use equality because the error message contains timing information that is not test-independent.

Do we need timing information? What is actually used there?

The full stderr of mozjs for successful test looks like a.2asm.js:warning: Successfully compiled asm.js code (total compilation time 4ms; caching disabled by missing command-line arguments). The only part the test cares about is Successfully compiled asm.js code. I don't know of a way to stop the timing information from being emitted, which would allow us to use equality here.

Oh, I see. Yeah, that's hard to remove. Looks good then, maybe add a comment though to say why we just check for the subset (if there isn't one already).

kripken · 2017-07-31T20:26:22Z

-      table.resize(tableSize);
-      for (size_t j = 0; j < tableSize; j++) {
-        table[j] = fromName(name);
+  for (Table::Segment &seg : wasm->table.segments) {


& should be on the type, not the var

kripken · 2017-07-31T20:29:59Z

 function add(x, y) {
-  x = x | 0;
-  y = y | 0;
+  x = (x | 0)


this looks worse than before - the parens are not needed, and the ; is not strictly needed but is safer to emit. it's actually possible some asm.js compilers depend on the lack of parens and the existence of the ';', I'm not sure - the spec can be read as assuming the very canonical form x = x | 0; (up to whitespace)

These changes are a result of STAT nodes being removed from the code base. I will put them back in to fix this.

We took out STAT nodes as an optimization, though. I think we should emit ;s without needing an AST node for them. It should be easy as discussed before, to just emit one after each element in a block (and loop body, and if arms).

The loop body and if arms are already taken of, but when I tried making it add a ; after each element of a block, I got a lot of ugly double semicolons and semicolons on their own line. Is that preferable to leaving them out or reinstating STAT nodes? I have a separate simple fix for the parentheses problem.

It's safer to leave extra semicolons in (no semicolons can run into automatic semicolon insertion nonsense in js...). Let's do that, and maybe figure out how to remove then later.

dcodeIO · 2017-08-01T09:49:43Z

Would be quite useful in binaryen.js as well, i.e. as Module#emitAsm.

tlively · 2017-08-02T21:00:52Z

@kripken The semicolons should be good enough for now. Is there anything else you see that should be fixed?

kripken · 2017-08-03T03:19:13Z

lgtm, thanks.

Get wasm2asm building again

279fdee

Updates CMakeLists.txt to have wasm2asm built by default, updates wasm2asm.h to account for recent interface changes, and restores JSPrinter functionality.

tlively added 2 commits July 21, 2017 13:36

Merge branch 'master' of github.com:WebAssembly/binaryen into binarye…

e358263

…n-local

Add missing header

262b51d

jgravelle-google reviewed Jul 24, 2017

View reviewed changes

cleanups

d792ee1

jgravelle-google reviewed Jul 24, 2017

View reviewed changes

tlively added 4 commits July 26, 2017 07:44

Update for AssignName refactor, add visitDrop

662f376

Implement splice for array values

891581b

Enable initial wasm2asm testing

b49dda4

Disable wasm2asm in check.py

899def2

jgravelle-google reviewed Jul 27, 2017

View reviewed changes

Clean up wasm2asm testing

5b85fd0

tlively added 2 commits July 27, 2017 13:23

Make test script finish successfully

9fc9144

Please the python linter

a072c3d

jgravelle-google reviewed Jul 27, 2017

View reviewed changes

Re-enable flake8 E127

19bce00

tlively added 2 commits July 28, 2017 14:04

Print semicolons after statements in blocks

9abb406

Clean up printing in wasm2asm test

cb57f4f

kripken reviewed Jul 31, 2017

View reviewed changes

Cleanups and semicolons for condition arms

e75d546

Move ampersand

b803ad9

tlively added 3 commits August 1, 2017 17:15

Prettify semicolon emission

77434a0

Add comment explaining subset test

281b6e0

Tiny cleanup

c5ba2ea

kripken merged commit ffd9a72 into WebAssembly:master Aug 3, 2017

tlively deleted the binaryen-local branch April 24, 2020 22:45

Conversation

tlively commented Jul 21, 2017

Uh oh!

tlively commented Jul 21, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kripken commented Jul 22, 2017

Uh oh!

tlively commented Jul 24, 2017

Uh oh!

kripken commented Jul 24, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tlively Jul 24, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jgravelle-google left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kripken commented Jul 24, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tlively commented Jul 27, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jgravelle-google commented Jul 27, 2017

Uh oh!

tlively commented Jul 27, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kripken commented Jul 28, 2017

Uh oh!

tlively commented Jul 28, 2017

Uh oh!

kripken left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tlively commented Jul 21, 2017 •

edited

Loading

tlively Jul 24, 2017 •

edited

Loading

tlively commented Jul 27, 2017 •

edited

Loading

tlively commented Jul 27, 2017 •

edited

Loading