Add shared memories by dschuff · Pull Request #1069 · WebAssembly/binaryen

dschuff · 2017-06-27T00:58:34Z

For now, just IR, wast, and binary support.

dschuff · 2017-06-27T17:05:03Z

src/wasm/wasm-binary.cpp

-  uint32_t flags = hasMaximum ? 1 : 0;
+void WasmBinaryWriter::writeResizableLimits(Address initial, Address maximum,
+                                            bool hasMaximum, bool shared) {
+  uint32_t flags = (uint32_t) hasMaximum | (uint32_t) shared << 1;


@binji This uses the 2nd bit as the "shared" bit, i.e
0x03 is the flags value for shared. https://github.com/WebAssembly/threads/blob/master/proposals/threads/Overview.md#spec-changes says it's 0x11 but I'm wondering if you meant 0b11?

kripken · 2017-06-27T18:05:08Z

What are shared memories? Is this something new in wasm? Where are they supported?

dschuff · 2017-06-27T18:13:15Z

@kripken as specified in the threading proposal https://github.com/WebAssembly/threads

dschuff · 2017-06-27T18:22:56Z

Specifically https://github.com/WebAssembly/threads/blob/master/proposals/threads/Overview.md

kripken · 2017-06-27T18:27:55Z

src/wasm-binary.h

  // gets a name in the combined function import+defined function space
  Name getFunctionIndexName(Index i);
-  void getResizableLimits(Address& initial, Address& max, Address defaultIfNoMax);
+  void getResizableLimits(Address& initial, Address& max, bool& shared, Address defaultIfNoMax);


why not put it at the end? seems more natural there at first glance, maybe there's something I'm missing?

The first 3 parameters are out-parameters, and the last one is an in-parameter, so I wanted to keep them together.

I see. lgtm

kripken · 2017-06-27T18:28:40Z

src/wasm/wasm-binary.cpp

  auto flags = getU32LEB();
  initial = getU32LEB();
  bool hasMax = flags & 0x1;
+  bool is_shared = flags & 0x2;


kripken · 2017-06-27T18:30:15Z

Thanks, now I understand the context here.

sbc100 · 2017-06-27T19:04:48Z

auto_update_tests.py

+    f.close()
+    cmd = ['mozjs', 'a.js']
+    out = run_command(cmd, stderr=subprocess.STDOUT)
+    open(os.path.join('test', 'binaryen.js', s + '.txt'), 'w').write(out)


Should we not be consistently using "with ... open as:" ?

Yes, we probably should, but it's not common enough in this codebase. I'll fix this instance though since I'm touching it anyway.

sbc100 · 2017-06-27T19:31:02Z

test/memory-shared.wast

+(module
+ (memory $0 23 256 shared)
+)
+


Why do some of these have a trailing newline?

I think it's because wasm-dis prints an extra newline, whereas wasm-opt --print does not. Should maybe fix that in another PR.

jgravelle-google · 2017-06-27T20:03:15Z

auto_update_tests.py

+    f.write(open(os.path.join('bin', 'binaryen.js')).read())
+    f.write(open(os.path.join('test', 'binaryen.js', s)).read())
+    f.close()
+    cmd = ['mozjs', 'a.js']


Should probably be cmd = [MOZJS, 'a.js']?

OK, but If I make any more changes to this code I'm going to have to go build mozjs just so I can test it =-O

jgravelle-google · 2017-06-27T20:10:37Z

src/wasm/wasm-binary.cpp

-  uint32_t flags = hasMaximum ? 1 : 0;
+void WasmBinaryWriter::writeResizableLimits(Address initial, Address maximum,
+                                            bool hasMaximum, bool shared) {
+  uint32_t flags = (uint32_t) hasMaximum | (uint32_t) shared << 1;


Instead of two bools in the signature, would it make sense to have a MemoryFlags enum?

enum MemoryFlags { memoryHasMaximum = 1 << 0, memoryIsShared = 1 << 1 };

As it is now we're basically inlining the knowledge of the flag layout, which is probably less obvious in the future.
Failing that a link in a comment to the threads proposal might be good, because otherwise that's implicit knowledge.

I dunno, the point of having the bools in the signature is to abstract away the encoding details from the caller (as with initial and maximum). But having bits explicit is good; I moved that to BinaryConsts

jgravelle-google · 2017-06-27T20:13:19Z

src/wasm/wasm-binary.cpp

      case ExternalKind::Table: {
        o << S32LEB(BinaryConsts::EncodedType::AnyFunc);
-        writeResizableLimits(wasm->table.initial, wasm->table.max, wasm->table.max != Table::kMaxSize);
+        writeResizableLimits(wasm->table.initial, wasm->table.max, wasm->table.max != Table::kMaxSize, false);


Instead of calling naked falses, would defaulting the param to false make sense?

Hm I actually like the pattern of
writeResizeableLimits(..., .., ..., /*shared=*/false) here.

That is also pretty reasonable

jgravelle-google · 2017-06-27T20:20:27Z

auto_update_tests.py

    cmd = ['mozjs', 'a.js']
    out = run_command(cmd, stderr=subprocess.STDOUT)
-    open(os.path.join('test', 'binaryen.js', s + '.txt'), 'w').write(out)
+    with open(os.path.join('test', 'binaryen.js', s + '.txt'), 'w') as o: o.write(out)


Nit: o.write(out) on its own line

Every other instance of with in this file uses this pattern. I think it may have been a compromise between `you should use with instead of just open().write!' and 'but verbosity!'

Yeah consistency > perfection. Seems fine

dschuff added 8 commits June 26, 2017 14:47

Add IR and wast syntax for shareable memories

b573ea4

Fix import parsing too

9231f58

update spec

6025ded

fix test updater, auto-update tests

7bd8e6c

fix flake8

4091253

dont print notshared, add tests

302c6da

Add binary support

7d7bc0d

fix whitespace

6a9af72

dschuff requested review from binji and jgravelle-google June 27, 2017 17:02

dschuff commented Jun 27, 2017

View reviewed changes

binji approved these changes Jun 27, 2017

View reviewed changes

Add missing test file

42034cb

kripken reviewed Jun 27, 2017

View reviewed changes

sbc100 reviewed Jun 27, 2017

View reviewed changes

reviews

2b5e80b

jgravelle-google reviewed Jun 27, 2017

View reviewed changes

jgravelle reviews

c2ccba6

jgravelle-google approved these changes Jun 27, 2017

View reviewed changes

dschuff merged commit 21e08ee into master Jun 27, 2017

dschuff deleted the shared_mem branch June 27, 2017 21:24

dschuff mentioned this pull request Jul 10, 2017

Wasm threading/shared memory support #1085

Closed

10 tasks

Conversation

dschuff commented Jun 27, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kripken commented Jun 27, 2017

Uh oh!

dschuff commented Jun 27, 2017

Uh oh!

dschuff commented Jun 27, 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!

kripken commented Jun 27, 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!

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants