Initial implementation of "Memory64" proposal by aardappel · Pull Request #3130 · WebAssembly/binaryen

aardappel · 2020-09-14T20:35:10Z

Also includes a lot of new spec tests that eventually need to go into the spec repo.

aardappel · 2020-09-14T20:38:06Z

As I mentioned, this contains a lot of fixes to test scripts and such, which aren't strictly part of Memory64 of course, but were required for me to get all this up and running on Windows. Some could be pulled out into a PR to be merged before this one (since I can't test this functionality without them), but would be simpler to just merge them here.

dcodeIO · 2020-09-15T05:49:46Z

Took a look at the emcc test error fwiw and found that it is happening at

binaryen/test/binaryen.js/expressions.js

Lines 492 to 494 in bcc6f29

    
           theHost.type = binaryen.f64; 
        
           theHost.finalize(); 
        
           assert(theHost.type === binaryen.i32);

indicating that calling finalize() on a Host expression representing a MemoryGrow operation does not update the expression's type based on its operands etc., but keeps potentially invalid types.

aardappel · 2020-09-15T15:29:16Z

@dcodeIO thanks so much for that insight! I did change finalize to take an extra parameter, but at the time I thought this was ok because a) there were other types doing this and b) surely I'd get a compile error if it wasn't. How naive :P

aardappel · 2020-09-15T16:29:21Z

Ok, changed the way Host::finalize works and is called to satisfy the tests, but it is very broken. What really needs to happen is for it to use the current pointer size, which depends on the current memory/module, which is not accessible from most of these functions calling finalize. We could stick an is64 bool in the visitors calling this function, e.g. BinaryenExpressionFinalize would need to take a module argument, changing the JS API etc.

sbc100

Surprisingly small change (tests aside)! Even makes some things simpler.

lgtm % comments

dcodeIO · 2020-09-15T16:40:09Z

I remember that there has been the idea to refactor Host into MemorySize and MemoryGrow, matching the other Memory* expressions. Not sure how much sense it makes in context, but if something needs to be changed anyway, perhaps refactoring these and also make them take the current memory size as an argument upon construction might be helpful, so all the information necessary to finalize is present?

kripken

Nice work! Some initial comments here.

I think it would be worth splitting out the unrelated windows test improvements. I'd be fine with them all in one big "fix windows" PR.

aardappel · 2020-09-17T16:56:58Z

Ok, non-Memory64 related changes have been split into a separate PR, please review that one first, as it is required for me to be able to test this one: #3142

aardappel · 2020-09-17T19:13:38Z

Ok, rebased to not include the Windows/test changes, and most comments above resolved, please review.

tlively · 2020-09-17T21:29:40Z

Ok, changed the way Host::finalize works and is called to satisfy the tests, but it is very broken. What really needs to happen is for it to use the current pointer size, which depends on the current memory/module, which is not accessible from most of these functions calling finalize. We could stick an is64 bool in the visitors calling this function, e.g. BinaryenExpressionFinalize would need to take a module argument, changing the JS API etc.

Why is the current memory or module not accessible from functions calling finalize? Where are these callers?

aardappel · 2020-09-17T21:33:47Z

@tlively BinaryenExpressionFinalize is an example. It would need to take a BinaryenModuleRef arg, changing the JS interface etc.

tlively · 2020-09-17T21:40:51Z

That sounds fine to me. We will have to do something similar when we expose Poppy IR to the C/JS API because that changes block validation as well. Maybe we can introduce a ValidationConfig object in the C/JS API where we can put all this validation context.

aardappel · 2020-09-17T22:39:22Z

Omg, CI beast has been slain!

tlively · 2020-09-17T22:46:00Z

@aardappel can you push additional commits instead of force pushing? It makes it hard to figure out what has been changed from version to version.

sbc100 · 2020-09-17T22:55:31Z

@aardappel can you push additional commits instead of force pushing? It makes it hard to figure out what has been changed from version to version.

I know you are not talking to me here but as someone who uses a "rebase early and squash often" approach in all my repos I would find this request hard.

I suppose I could still do what I want with my local branch and then every time I want to push a change to github I could just push the delta between what I have in my branch and what github currently has? i.e. have my work branch and the github branch be basically unrelated. I guess I could write a script for that.

aardappel · 2020-09-17T22:57:52Z

@tlively they are often very tiny formatting changes and what not, and make it harder for me to see locally what's all in a PR.

tlively · 2020-09-17T23:47:29Z

Ok, I don't want to mess with your normal workflow too much. GitHub's "changes since last review" feature is great, but I wish it worked with with rebases. When I need to see everything that's in a PR I usually just do git diff master (or just look at GitHub).

aardappel · 2020-09-18T00:04:10Z

@tlively I use a local git tool (Fork).

kripken · 2020-09-18T16:41:28Z

-          value64 + offset64 <= uint64_t(std::numeric_limits<int32_t>::max())) {
-        last->value = Literal(int32_t(value64 + offset64));
+      if (getModule()->memory.is64) {
+        last->value = Literal(int64_t(value64 + offset64));


I think the danger is still relevant in memory64, if it wraps doesn't it trap? (what does the memory64 spec do there?)

I believe we haven't decided on that yet, see: WebAssembly/memory64#3

I'd be happy to add any checks.. but for now doing the simplest thing seems fine to me.

Oh, I didn't realize the state of the spec... sounds fine then, but maybe add a comment that this is in flux?

kripken · 2020-09-18T16:49:35Z

+  bool is64;

 public:
  Builder(MixedArena& allocator) : allocator(allocator) {}


the first constructor looks dangerous, as it won't set is64. we should maybe remove that constructor entirely (separately from this PR). meanwhile, it would be good to at least check in makeConstPtr if we know the memory size.

It now takes a module, and made sure all users of Builder always supply a valid module

kripken · 2020-09-18T16:50:28Z


 class Builder {
  MixedArena& allocator;
+  bool is64;


I'd suggest storing a Module& here instead of is64. is64 duplicates info that might get stale, if we add a pass to convert from 64 to 32 or vice versa etc.

Or this could be a Module* defaulting to nullptr which would help with the issue from a few lines down.

All callers have a valid module, so wasn't necessary to make it a *

This is going to be inconsistent with Poppy IR, where individual functions rather than the module are the source of truth for the extra validation context. I would have preferred taking the risk of stale data. Most builders are not long-lived, anyhow, and most passes will not be changing the validation context in any way. I just uploaded #3144, but I would actually prefer to keep the allocator and use an is64 bool. @kripken, wdyt?

I'm confused, why would Poppy IR change the relationship of functions to the module?

Anyhow it's late on Friday, let's discuss this in depth on Monday?

Also includes a lot of new spec tests that eventually need to go into the spec repo

kripken

Very nice!

I didn't go through each spec test manually. I figure we'll go through those carefully upstream anyhow. Also once we get fuzzing integration for wasm64 that should be a good check on them.

kripken · 2020-09-18T22:32:44Z

-  Builder(MixedArena& allocator) : allocator(allocator) {}
-  Builder(Module& wasm) : allocator(wasm.allocator) {}
+  Builder(MixedArena& allocator, Module& wasm)
+    : allocator(allocator), wasm(wasm) {}


As a followup I think we can remove this constructor. The allocator should always be from the wasm, so there is no need to pass an allocator manually.

I did see at least one use where it seemed that a single allocator was used for potentially processing multiple modules, and I didn't know how intentional that was, so I left it in.

kripken · 2020-09-18T22:33:04Z

    return ret;
  }
+  Const* makeConstPtr(uint64_t val) {
+    return makeConst(Literal::makeFromUInt64(val, wasm.memory.indexType));


I like how this looks!

kripken · 2020-09-18T22:34:33Z


+  Type ptrType = Type::i32;
+
+  void make64();


In general we don't like adding methods to these classes, to keep them minimalistic. But I can't think of a better pattern, and these are rarely-used anyhow, so sgtm.

I tried adding an argument to the constructor, but that didn't work with the alloc template, so did this instead to keep it simple.

Builders gained a `Module` field in WebAssembly#3130 because they now require extra context to properly finalize some Expressions. Since modules contain allocators, the old allocator field on the builder became redundant after that change. This PR removes the redundant allocator field.

Builders gained a `Module` field in #3130 because they now require extra context to properly finalize some Expressions. Since modules contain allocators, the old allocator field on the builder became redundant after that change. This PR removes the redundant allocator field.

aardappel requested review from kripken and sbc100 September 14, 2020 20:35

sbc100 reviewed Sep 15, 2020

View reviewed changes

Comment thread CMakeLists.txt

Comment thread CMakeLists.txt Outdated

Comment thread src/tools/execution-results.h

Comment thread src/wasm-binary.h Outdated

Comment thread src/wasm/CMakeLists.txt Outdated

Comment thread src/wasm/wasm-validator.cpp Outdated

kripken reviewed Sep 15, 2020

View reviewed changes

Comment thread scripts/test/wasm2js.py

Comment thread src/literal.h Outdated

Comment thread src/passes/ExtractFunction.cpp Outdated

Comment thread src/passes/InstrumentMemory.cpp Outdated

Comment thread src/wasm.h Outdated

Comment thread src/wasm.h Outdated

dcodeIO mentioned this pull request Sep 16, 2020

Refactor Host expression to MemorySize and MemoryGrow #3137

Merged

kripken reviewed Sep 18, 2020

View reviewed changes

Initial implementation of "Memory64" proposal

21ebdf4

Also includes a lot of new spec tests that eventually need to go into the spec repo

kripken approved these changes Sep 18, 2020

View reviewed changes

aardappel merged commit 3b4cb93 into WebAssembly:master Sep 18, 2020

tlively mentioned this pull request Sep 19, 2020

Remove redundant allocator in Builder #3144

Merged

MaxGraey mentioned this pull request Sep 20, 2020

Fuzzer: binaryen interpreter and d8 comparison error #3149

Closed

aardappel mentioned this pull request Sep 28, 2020

New spec tests WebAssembly/memory64#8

Merged

Conversation

aardappel commented Sep 14, 2020

Uh oh!

aardappel commented Sep 14, 2020

Uh oh!

dcodeIO commented Sep 15, 2020

Uh oh!

aardappel commented Sep 15, 2020

Uh oh!

aardappel commented Sep 15, 2020

Uh oh!

sbc100 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dcodeIO commented Sep 15, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kripken left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

aardappel commented Sep 17, 2020

Uh oh!

aardappel commented Sep 17, 2020

Uh oh!

tlively commented Sep 17, 2020

Uh oh!

aardappel commented Sep 17, 2020

Uh oh!

tlively commented Sep 17, 2020

Uh oh!

aardappel commented Sep 17, 2020

Uh oh!

tlively commented Sep 17, 2020

Uh oh!

sbc100 commented Sep 17, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aardappel commented Sep 17, 2020

Uh oh!

tlively commented Sep 17, 2020

Uh oh!

aardappel commented Sep 18, 2020

Uh oh!

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!

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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kripken left a comment

dcodeIO commented Sep 15, 2020 •

edited

Loading

sbc100 commented Sep 17, 2020 •

edited

Loading