Poppy IR wast parsing and validation #1

tlively · 2020-09-04T03:04:39Z

Adds and IR Profile to each function so the validator can determine
which validation rules to apply and adds an flag to have the wast
parser set the profile to Poppy for testing purposes.

First, adds an explicit destructor call to fix a memory leak in `Literal::operator=` in which existing `ExceptionPackage`s would be silently dropped. Next, changes `Literal::getExceptionPackage` to return the `ExceptionPackage` by value to avoid a use-after-free bug in the interpreter that was surfaced by the new destructor call. A future improvement would be to switch to using `std::variant`. Fixes WebAssembly#3087.

* Make `Literal::type` immutable to guarantee that we do not lose track of `Literal::exn` by changing the literal's type * Add an assert to guarantee that we don't create `exnref` literals without an `ExceptionPackage` (for now) * Enforce rvalue reference when creating an `exnref` Literal from a `std::unique_ptr<ExceptionPackage>`, avoiding a redundant copy by means of requiring `std::move`

Fixes `DataFlowOpts` leaking allocated temporary functions created to precompute an expression as their body. Reusing the `body` afterwards is fine since expressions are arena allocated separately.

See emscripten-core/emscripten#9039 (comment) The valid stack area is a region [A, B] in memory. Previously we just checked that new stack positions S were S >= A, which prevented us from growing too much (the stack grows down). But that only worked if the growth was small enough to not overflow and become a big unsigned value. This PR makes us check the other way too, which requires us to know where the stack starts out at. This still supports the old way of just passing in the growth limit. We can remove it after the roll. In principle this can all be done on the LLVM side too after emscripten-core/emscripten#12057 but I'm not sure of the details there, and this is easy to fix here and get testing up (which can help with later LLVM work). This helps emscripten-core/emscripten#11860 by allowing us to clean up some fastcomp-specific stuff in tests.

…embly#3089) We were careful not to minify those, as well as the stack pointer, which makes sense in dynamic linking. But we don't run this pass in dynamic linking anyhow - we need the proper names of symbols in that case. So this was not helping us, and was just a leftover from an early state. This both a useful optimization and also important for WebAssembly#3043, as the wasm backend exports the table as __indirect_function_table - a much longer name than emscripten's table. So just changing to that would regress code size on small projects. Once we land this, the name won't matter as it will be minified anyhow.

…#3095) Fixes `LegalizeJSInterface::makeLegalStub` forgetting to `delete` stub Functions that turned out to be not needed. Now checks whether a stub is needed and otherwise skips creating the redundant stub right away.

Fixes `Wasm2JSBuilder` leaking a temporary `Function` (`WASM_FETCH_HIGH_BITS`) in `Wasm2JSBuilder::processWasm`. The function is created to be converted to JS, but is not actually part of the module, so it either needs to be cleaned up separately or be added to the module. This PR does the latter in case it is useful.

…y#3101) When minimizing wasm changes, leave it as __indirect_function_table which is what LLVM emits. This also removes the renaming of the memory. That was never needed as LLVM already emits "memory" there. See WebAssembly#3043

BranchSeekerCache caches the set of branches in a node + its children, and helps compute new results by looking in the cache and using data for the children. This avoids quadratic time in the common case of a post-walk on a tower of nested blocks which is common in a switch. Fixes WebAssembly#3090 . On the testcase there this pass goes from over a minute to less than a second.

tlively · 2020-09-04T03:05:50Z

FYI @kripken @aheejin this is a followup to WebAssembly#3083, but I will have to reopen this targeting the main repo once WebAssembly#3083 lands. Unfortunately this is the best I can do for now because GitHub does not support stacked PRs.

Split that mode into an option to check for loops (which indicate a function is "heavy") and a constant check for having calls. The case of calls is different as we would need more logic to avoid infinite recursion if we are willing to inling functions with calls. Practically, this renames allowHeavyweight to allowFunctionsWithLoops.

kripken · 2020-09-05T22:31:15Z

src/ir/stack-utils.h

@@ -75,6 +75,9 @@ namespace StackUtils {
 // Iterate through `block` and remove nops.
 void removeNops(Block* block);

+// Whether `expr` may be unreachable in Poppy IR


This comment is ambiguous. From the impl it looks like a block/loop etc. gets "true", so I guess the meaning is "not exited normally", and not "unreachable / causes polymorphic stack"?

kripken · 2020-09-05T22:32:02Z

src/tools/tool-options.h

+      .add(
+        "--poppy",
+        "",
+        "Parse wast files as Poppy IR",


Is the option to parse as poppy just for testing?

Yes, that's the intention. Users shouldn't have to think about different IRs at all. I can change the flag name and description to make that clear. How does --experimental-poppy sound?

Yes, "experimental" in the flag sounds good. 👍

kripken · 2020-09-05T23:09:45Z

src/wasm/wasm-validator.cpp

@@ -247,6 +248,15 @@ struct FunctionValidator : public WalkerPass<PostWalker<FunctionValidator>> {
 public:
  // visitors

+  void validatePoppyExpression(Expression* curr);
+  static void visitExpression(FunctionValidator* self, Expression** currp) {


empty line after previous declaration.

kripken · 2020-09-06T14:46:12Z

src/wasm/wasm-validator.cpp

@@ -277,6 +289,7 @@ struct FunctionValidator : public WalkerPass<PostWalker<FunctionValidator>> {
    if (curr->is<Loop>()) {
      self->pushTask(visitPreLoop, currp);
    }
+    self->pushTask(visitExpression, currp);


How about checking here if getFunction() and the profile is Poppy? Can then rename to visitPoppyExpression.

kripken · 2020-09-06T14:49:53Z

src/wasm/wasm-validator.cpp

+    case IRProfile::Poppy:
+      validatePoppyBlockElements(curr);
+      break;
+  }


Are blocks really the only place where we have different validation - that is, in all other cases Poppy validation is just additional validation on top of the normal rules? If so it would be good to document that, that's a nice property!

Yep! As designed :) I can add a note about that to the docs that are currently in stack-utils.h.

Implement and test utilities for manipulating and analyzing a new stacky form of Binaryen IR that is able to express arbitrary stack machine code. This new Poppy IR will eventually replace Stack IR, and new optimization passes will be built with these utilities. See WebAssembly#3059.

Adds and IR Profile to each function so the validator can determine which validation rules to apply and adds an flag to have the wast parser set the profile to Poppy for testing purposes.

tlively and others added 10 commits September 1, 2020 15:55

Fix DataFlowOpts leaking temporary Functions (WebAssembly#3093)

ef7ab77

Fixes `DataFlowOpts` leaking allocated temporary functions created to precompute an expression as their body. Reusing the `body` afterwards is fine since expressions are arena allocated separately.

Remove old stack function from StackCheck (WebAssembly#3100)

132c72b

kripken reviewed Sep 6, 2020

View reviewed changes

dcodeIO and others added 3 commits September 6, 2020 21:21

Upgrade to C++17 (WebAssembly#3103)

775363a

Poppy IR wast parsing and validation

d14bea8

Adds and IR Profile to each function so the validator can determine which validation rules to apply and adds an flag to have the wast parser set the profile to Poppy for testing purposes.

tlively force-pushed the stackify-2-wast-parsing branch from dd73ab2 to d14bea8 Compare September 8, 2020 02:37

tlively closed this Sep 8, 2020

tlively reopened this Sep 9, 2020

tlively closed this Sep 9, 2020

tlively deleted the stackify-2-wast-parsing branch September 9, 2020 20:01

kripken mentioned this pull request Sep 9, 2020

Poppy IR wast parsing and validation WebAssembly/binaryen#3105

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Poppy IR wast parsing and validation #1

Poppy IR wast parsing and validation #1

Uh oh!

tlively commented Sep 4, 2020

Uh oh!

tlively commented Sep 4, 2020

Uh oh!

kripken Sep 5, 2020

Uh oh!

kripken Sep 5, 2020

Uh oh!

tlively Sep 6, 2020

Uh oh!

kripken Sep 8, 2020

Uh oh!

kripken Sep 5, 2020

Uh oh!

kripken Sep 6, 2020

Uh oh!

kripken Sep 6, 2020

Uh oh!

tlively Sep 6, 2020

Uh oh!

Uh oh!

Poppy IR wast parsing and validation #1

Poppy IR wast parsing and validation #1

Uh oh!

Conversation

tlively commented Sep 4, 2020

Uh oh!

tlively commented Sep 4, 2020

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!

Uh oh!