WIP: Support for mutable global imports/exports by jayphelps · Pull Request #1644 · WebAssembly/binaryen

jayphelps · 2018-08-26T22:08:59Z

Wanted to open the conversation around adding support for mutable global imports/exports so I made what I think are most of the necessary changes--I'm probably missing some parts in one of the tools.

I'm also ran into some issues with check.py and auto_update_tests.py on MacOS but we can discuss those in #1185

No worries if this PR isn't merged, I mostly did the work to unblock myself for now 🌮

jayphelps · 2018-08-26T22:11:40Z

src/wasm.h

  ExternalKind kind;
  Name functionType; // for Function imports
  Type globalType; // for Global imports
+  bool mutable_; // for Global imports


This feels dirty but seemed like a simple solution without deciding to completely rethink about imports/globals are represented

Seems reasonable to me. Might be nice to eventually think about a union or other cleaner approach, but Imports are not that common, so I'm not worried about it.

kripken

Thanks! Good start, everything looks correct to me aside from the one comment below on reading of SetGlobals.

kripken · 2018-08-26T22:15:21Z

src/wasm/wasm-binary.cpp

+    return;
+  }
+  throwError("bad set_global");
  curr->finalize();


I may be missing something, but I don't think the code added to this function is needed - the type of SetGlobal is always none, as it does not return a value.

You're not missing something, I was! You are correct and it actually explains why some codegen around the tests were adding in a drop. Great catch.

kripken · 2018-08-26T22:20:14Z

Otherwise, we have a FeatureSet concept at the top of wasm.h, currently with a flag for atomics. Not sure if we intend to add a flag for every new feature, or if mutable global imports in particular - @dschuff, thoughts?

Other things we'll need here eventually:

wasm-interpreter.h support.
Tests.
Fuzzer support.
Optimizer support, if we decide it's worth it.

kripken · 2018-08-26T22:26:44Z

Oh, and about tests: auto_update_tests.py runs the tests and updates their outputs. So you can add a test input, run that script, and then git add and commit the output. Then check.py will check that output is the same when it is run.

For this feature, we might want to add the spec tests, but we haven't in a while and I'm not sure it's worth the effort. Otherwise, can add test/mutable-global.wast with some code, and then the test suite will convert it to binary and back, so if you run the auto script it will generate an output like test/mutable-global.wast.fromBinary or something like that.

jayphelps

Added a test, and some fixes that the test revealed, however it also caused some other tests to change to mutable and it's not clear why. I'll continue digging but if you happen to know immediately why lmk.

jayphelps · 2018-08-26T22:49:04Z

src/passes/Print.cpp

+          o << "(mut " << printType(curr->globalType) << ")";
        } else {
-          o << printType(curr->globalType) << ' ';
+          o << printType(curr->globalType);


The test revealed that I had incorrectly added spaces here

jayphelps · 2018-08-26T22:49:57Z

src/wasm/wasm-s-parser.cpp

    im->kind = ExternalKind::Global;
    im->globalType = type;
-    if (wasm.getImportOrNull(im->name)) throw ParseException("duplicate import", s.line, s.col);
+    im->mutable_ = mutable_;


testing revealed I missed adding the support to this file.

jayphelps · 2018-08-26T22:52:08Z

test/empty.fromasm.clamp.no-opts

 (import "env" "table" (table 0 0 anyfunc))
- (import "env" "memoryBase" (global $memoryBase i32))
- (import "env" "tableBase" (global $tableBase i32))
+ (import "env" "memoryBase" (global $memoryBase (mut i32)))


@kripken It's not clear why all these changed to mutable. Perhaps this is revealing a bug I introduced?

Oh! Is it because those asmjs globals were actually mutable but previously we couldn’t represent that?

Interesting - I think that's a coincidence actually, but a good one here as it pointed out something useful :) What I think is going on is that we forgot an initial value for bool mutable_; (unlike the other fields near it, it doesn't have a default value) - we should set that to bool mutable_ = false;.

It's also a good question what we should do for asm.js imports. Yeah, in asm.js they are mutable, and to work around that in wasm we created a mutable global with the initial value of the import, then we just use that global from then on. In theory we could simplify that to use a mutable import, however, I think it would be more complicated than it would be worth since we do want the old form supported too (and also a mutable import may have perf overhead).

jayphelps · 2018-08-29T01:16:44Z

Fixed bool mutable_ = false; default, rebuilt tests, however I needed to update the testsuite/spec files to pick up mutual global support and in doing so it opened a big old can of worms with incompatibilities 😱. I also couldn't find an intermediate commit we can use to ship this. I've been digging through them for a while now, so far biggest ones are the renaming of memory instructions (e.g. grow_memory -> memory.grow) and numerous tests now mix stack/sexprs which binaryen doesn't support. Gonna see how far I get. If I'm not back in a few days send help ☠️🏳️

binji · 2018-08-29T01:34:27Z

and numerous tests now mix stack/sexprs which binaryen doesn't support.

You could use wat-desugar --fold to convert from flat format to folded format, if the flat format is not essential to the test. It would be a bit tedious, since it currently only works on individual modules, however.

jayphelps · 2018-08-29T01:46:38Z

@binji thanks for the tip--I really do appreciate it. Unfortunately wat-desugar strips comments and also removes some extraneous code that are part of the test (like deeply nested blocks). But lmk if I'm misunderstanding!

binji · 2018-08-29T02:37:14Z

Yes, it will remove comments (it'd be nice if the wabt parser kept them around for tools like this). It shouldn't remove blocks though; if you have a repro case I'd like to fix that.

But yeah, you can't just run it directly, but you could use it to help fold code rather than doing it by hand.

jayphelps · 2018-08-29T02:46:52Z

if you have a repro case I'd like to fix that

turns out I was looking at the git diff incorrectly. What actually happened is that it put the export outside instead of inline--my eye saw the export "deep" and thought the code was removed. Sorry about that!

(func (export "deep") (result i32)
    (block (result i32) (block (result i32)

        (; etc...I removed the rest for comment readability ;)

                                        (block (result i32) (block (result i32)
                                          (call $dummy) (i32.const 150)
                                        ))
                                      ))
                                    ))
                                  ))
                                ))
                              ))
                            ))
                          ))
                        ))
                      ))
                    ))
                  ))
                ))
              ))
            ))
          ))
        ))
      ))
    ))
  )

(func (;5;) (result i32)
    (block (result i32)  ;; label = @1
      (block (result i32)  ;; label = @2

        (; etc...I removed the rest for comment readability ;)

                                                                            (block (result i32)  ;; label = @37
                                                                              (block (result i32)  ;; label = @38
                                                                                (call $dummy)
                                                                                (i32.const 150))))))))))))))))))))))))))))))))))))))))
  (export "deep" (func 5))

binji · 2018-08-29T02:57:55Z

What actually happened is that it put the export outside instead of inline

Oh, there should be an option for that, but it looks like it's only in wasm2wat and not wat-desugar. 😅

kripken · 2018-08-29T14:55:50Z

however I needed to update the testsuite/spec files to pick up mutual global support and in doing so it opened a big old can of worms with incompatibilities

Ah yes, I was worried about that. It might not be worth the effort - I think it would be reasonable instead to just get the relevant mutable global tests directly. Specifically, we can replace the git submodule with just checked-in files (since we've diverged from upstream anyhow) of the current state, then add that one file (assuming it's in a new file).

sbc100 · 2018-11-29T02:14:41Z

Do you still want to land this?

I'd would like to see mutable global support in binaryen.

This picks up from #1644 and indeed borrows the test case from there.

sbc100 · 2018-11-30T16:49:05Z

I think this should be covered now by #1785 and

jayphelps · 2018-11-30T17:06:04Z

Thanks @sbc100! I had ran into a blocker with the spec test suite but if it passed for you it must have since been resolved. Woot woot!

jayphelps commented Aug 26, 2018

View reviewed changes

kripken reviewed Aug 26, 2018

View reviewed changes

Support for mutable global imports/exports

7231ff8

jayphelps force-pushed the mut-global branch from 4a88b1c to 7231ff8 Compare August 26, 2018 22:21

jayphelps commented Aug 26, 2018

View reviewed changes

WIP addition fixes for mutable global imports and tests

8a0fb7a

jayphelps force-pushed the mut-global branch from 5933954 to 8a0fb7a Compare August 27, 2018 03:50

dschuff mentioned this pull request Aug 28, 2018

Global and function import model #1649

Closed

jayphelps mentioned this pull request Aug 29, 2018

WIP rename memory instructions #1652

Open

dschuff mentioned this pull request Sep 7, 2018

Wasm threading/shared memory support #1085

Closed

10 tasks

jayphelps mentioned this pull request Sep 11, 2018

Check-in the test/spec suite instead of as submodule #1677

Merged

sbc100 added a commit that referenced this pull request Nov 29, 2018

Add support for a mutable globals as a Feature

016b22e

This picks up from #1644 and indeed borrows the test case from there.

sbc100 mentioned this pull request Nov 29, 2018

Add support for a mutable globals as a Feature #1785

Merged

sbc100 added a commit that referenced this pull request Nov 30, 2018

Add support for a mutable globals as a Feature

95570fa

This picks up from #1644 and indeed borrows the test case from there.

sbc100 added a commit that referenced this pull request Nov 30, 2018

Add support for a mutable globals as a Feature (#1785)

f16a760

This picks up from #1644 and indeed borrows the test case from there.

sbc100 closed this Nov 30, 2018

Conversation

jayphelps commented Aug 26, 2018 • 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!

Choose a reason for hiding this comment

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!

kripken commented Aug 26, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kripken commented Aug 26, 2018

Uh oh!

jayphelps 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!

jayphelps commented Aug 29, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

binji commented Aug 29, 2018

Uh oh!

jayphelps commented Aug 29, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

binji commented Aug 29, 2018

Uh oh!

jayphelps commented Aug 29, 2018

Uh oh!

binji commented Aug 29, 2018

Uh oh!

kripken commented Aug 29, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sbc100 commented Nov 29, 2018

Uh oh!

sbc100 commented Nov 30, 2018

Uh oh!

jayphelps commented Nov 30, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

jayphelps commented Aug 26, 2018 •

edited

Loading

kripken commented Aug 26, 2018 •

edited

Loading

jayphelps commented Aug 29, 2018 •

edited

Loading

jayphelps commented Aug 29, 2018 •

edited

Loading

kripken commented Aug 29, 2018 •

edited

Loading