Initial implementation of "Memory64" proposal #3130

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

CMakeLists.txt

src/tools/execution-results.h

src/wasm-binary.h

src/wasm/CMakeLists.txt

src/wasm/wasm-validator.cpp

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.

scripts/test/wasm2js.py

src/literal.h

src/passes/ExtractFunction.cpp

src/passes/InstrumentMemory.cpp

src/wasm.h

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).

src/literal.h

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

src/passes/OptimizeInstructions.cpp

-          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?

src/wasm-binary.h

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

src/wasm-builder.h

@@ -35,10 +35,11 @@ struct NameType {

 class Builder {
  MixedArena& allocator;
+  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

src/wasm-builder.h

@@ -35,10 +35,11 @@ struct NameType {

 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?

src/wasm.h

src/wasm/wasm-binary.cpp

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

src/wasm-builder.h

-  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

src/wasm-builder.h

@@ -486,6 +488,9 @@ class Builder {
    ret->finalize();
    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

src/wasm.h

@@ -1070,6 +1068,9 @@ class MemorySize : public SpecificExpression<Expression::MemorySizeId> {
  MemorySize() { type = Type::i32; }
  MemorySize(MixedArena& allocator) : MemorySize() {}

+  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

aardappel force-pushed the master branch 2 times, most recently from bf6d9b2 to 39a1262 Compare September 15, 2020 00:10

aardappel force-pushed the master branch 2 times, most recently from c9c9af3 to 1506bbd Compare September 15, 2020 16:24

aardappel force-pushed the master branch from 1506bbd to eba6dec Compare September 15, 2020 16:30

sbc100 reviewed Sep 15, 2020

View reviewed changes

aardappel force-pushed the master branch from eba6dec to a30d65d Compare September 15, 2020 16:48

kripken reviewed Sep 15, 2020

View reviewed changes

dcodeIO mentioned this pull request Sep 16, 2020

Refactor Host expression to MemorySize and MemoryGrow #3137

Merged

aardappel force-pushed the master branch 3 times, most recently from 1f1a95e to df53f58 Compare September 17, 2020 19:12

aardappel force-pushed the master branch 4 times, most recently from a3a7267 to 78edc1a Compare September 17, 2020 21:23

aardappel force-pushed the master branch 2 times, most recently from 29caea2 to e1398c7 Compare September 17, 2020 22:01

kripken reviewed Sep 18, 2020

View reviewed changes

aardappel force-pushed the master branch 2 times, most recently from cc1b2f2 to a66377a Compare September 18, 2020 20:01

Initial implementation of "Memory64" proposal

21ebdf4

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

aardappel force-pushed the master branch from a66377a to 21ebdf4 Compare September 18, 2020 20:18

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

Initial implementation of "Memory64" proposal #3130

Initial implementation of "Memory64" proposal #3130

Uh oh!

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!

dcodeIO commented Sep 15, 2020 •

edited

Loading

sbc100 commented Sep 17, 2020 •

edited

Loading