Poppify pass #3541

tlively · 2021-02-04T00:08:04Z

Adds a Poppify ("--poppify") pass for converting normal Binaryen IR to Poppy IR.
Like the existing construction of Stacky IR, Poppify depends on the
BinaryenIRWriter to drive the emitting of instructions in correct stack machine
order. As instructions are "emitted," Poppify replaces their children with pops
and collects them in a list. At the end of each scope, Poppify creates a block
containing all the collected instructions for that scope and injects that block
into the enclosing scope. All tuple globals and instructions dealing with tuples
are also expanded to remove all tuples from the program.

The validator currently fails to validate many valid Poppy IR patterns produced
in the tests, but fixing that is left as follow-on work to keep this PR focused
on the Poppify pass itself. For now the tests simply skip validation.

Add a utility for calculating the least upper bounds of two StackSignatures, taking into account polymorphic unreachable behavior. This will important in the finalization and validation of Poppy IR blocks, where a block is allowed to directly produce fewer values than the branches that target it carry if the difference can be made up for by polymorphism due to an unreachable instruction in the block. Also removes the Poppy IR documentation from stack-utils.h because that documentation is moved to the Stackify pass in WebAssembly#3541.

kripken

I believe I've already suggested calling this pass Poppify - seems clearer as we use the term "Poppy IR". What did we decide there?

kripken · 2021-02-04T02:11:46Z

src/passes/Stackify.cpp

@@ -0,0 +1,497 @@
+/*
+ * Copyright 2020 WebAssembly Community Group participants


Suggested change

* Copyright 2020 WebAssembly Community Group participants

* Copyright 2021 WebAssembly Community Group participants

kripken · 2021-02-04T02:13:21Z

src/passes/Stackify.cpp

+//   )
+//
+// Notice that the sequence of instructions in the block is now identical to the
+// sequence of instructions in raw WebAssembly. Also note that Poppy IR's


Suggested change

// sequence of instructions in raw WebAssembly. Also note that Poppy IR's

// sequence of instructions in a WebAssembly binary. Also note that Poppy IR's

kripken · 2021-02-04T02:13:35Z

src/passes/Stackify.cpp

+// sequence of instructions in raw WebAssembly. Also note that Poppy IR's
+// validation rules are largely additional on top of the normal Binaryen IR
+// validation rules, with the only exceptions being block body validation and
+// block unreahchability rules.


Suggested change

// block unreahchability rules.

// block unreachability rules.

kripken · 2021-02-04T02:14:16Z

src/passes/Stackify.cpp

+//     unreachable behavior in WebAssembly may have unreachable type. Blocks may
+//     be unreachable when they are not branch targets and when they have an
+//     unreachable child. Note that this means a block may be unreachable even
+//     if it would otherwise have a concrete type, unlike in Binaryen IR.


Please add an example of such a block - this is not clear to me.

kripken · 2021-02-04T02:27:03Z

src/passes/Stackify.cpp

+    auto types = func->getLocalType(curr->index);
+    const auto& elems = tupleVars[curr->index];
+    // Add the unconditional sets
+    for (size_t i = types.size() - 1; i >= 1; --i) {


I'm confused as to why 0 is special here. It seems like for tee you emit

set 3 set 2 set 1 tee 0 get 1 get 2 get 3

Why is that correct?

Oh, wait, I see it, nevermind...

Adds a Poppify ("--poppify") pass for converting normal Binaryen IR to Poppy IR. Like the existing construction of Stacky IR, Poppify depends on the BinaryenIRWriter to drive the emitting of instructions in correct stack machine order. As instructions are "emitted," Poppify replaces their children with pops and collects them in a list. At the end of each scope, Poppify creates a block containing all the collected instructions for that scope and injects that block into the enclosing scope. All tuple globals and instructions dealing with tuples are also expanded to remove all tuples from the program. The validator currently fails to validate many valid Poppy IR patterns produced in the tests, but fixing that is left as follow-on work to keep this PR focused on the Poppify pass itself. For now the tests simply skip validation.

tlively · 2021-02-04T07:30:02Z

I believe I've already suggested calling this pass Poppify - seems clearer as we use the term "Poppy IR". What did we decide there?

Oops, I had forgotten about that. I personally prefer "Stackify," but I also suspect that objectively you're right that "Poppify" is a better name and that I will get used to it quickly. I've gone ahead and renamed everything to use the poppify terminology.

kripken · 2021-02-04T21:11:27Z

src/passes/Poppify.cpp

+//     unreachable child. Note that this means a block may be unreachable even
+//     if it would otherwise have a concrete type, unlike in Binaryen IR. For
+//     example, this block could have unreachable type in Poppy IR but would
+//     have to have type i32 in Binaryen IR:


Thanks, now I understand the difference.

But why is this so? It's not clear to me why it has to be this way. (If it doesn't, removing differences would be good - but I assume there is a reason...)

TBH I don't remember the original concrete motivation for making this change in Poppy IR. Allowing more things to be unreachable allows DCE to be more aggressive, but I don't remember if this was an opportunistic change or if doing things the Binaryen IR way caused problems. I'll remove this documentation change from the PR for now and if it was there for a good reason I will find out (again) soon enough.

Oh wait, this documentation is not new in this PR; it already existed in stack-utils.h. Would it be ok with you if we revisited this point once things are working end-to-end? I'm concerned that changes we make in this half-built state will come back to bite us before we finish.

If it can speed things up I'm not opposed. But I'd like to understand how this can work, though. The point of Poppy IR is to model the stack machine format more closely, and that format doesn't have unreachable blocks. So shouldn't it be the opposite, that is, the example block here would always have type i32 in Poppy IR?

Or am I misunderstanding Poppy IR?

I'd say the point of Poppy IR is to make it easy to write optimizations and other transformations that take the stack machine structure of instructions into account. Part of that is necessarily modeling the stack machine syntax more closely, but unreachable types still give optimizations useful flexibility and information, so they are worth preserving in Poppy IR.

aheejin

Nice! The comments are mostly nits or questions:

aheejin · 2021-02-08T17:49:34Z

src/passes/Poppify.cpp

+//  1. Function bodies and children of control flow (except If conditions) must
+//     be blocks.
+//
+//  2. Blocks may have any Expressions except for Pops as children. The sequence


Is this an inherent characteristic of Poppy IR or it will change after we support blocks taking values?

Once we support blocks taking values, I expect that more blocks will have pop children. Right now catch blocks are somewhat limited because this binary cannot be directly expressed in Binaryen or Poppy IR because our we need to have some instruction to consume the pop of the caught value:

try i32 do i32.const 0 throw $e ;; throws i32 catch $e ;; i32 on stack end

Eventually I would like to implement that catch with this Poppy IR block:

(block (params i32) (results i32) (pop i32) )

Once we support blocks taking values, I expect that more blocks will have pop children.

Then this sounds like not an inherent characteristic of Poppy IR but a tentative restriction that will be removed later..? How about clarifying that in the comment?

Right now catch blocks are somewhat limited because this binary cannot be directly expressed in Binaryen or Poppy IR because our we need to have some instruction to consume the pop of the caught value:

try i32 do i32.const 0 throw $e ;; throws i32 catch $e ;; i32 on stack end

I'm not sure if I understand what you mean by 'limited' and we have to 'consume' the caught value. Given that we don't support block inputs at the moment, catch seems to be an exception, but why is it limited? And I think it is possible not to consume the caught value if we flow it out of try, as in your example?

I will update this comment to make clear that this restriction might be changed in the future.

I double checked and I was wrong about this example being invalid in Binaryen IR; sorry about that. There is no limitation in Binaryen IR after all. Right now the validator does improperly reject the Poppy IR version of that example, but I will fix that in a follow-up along with other validation issues with Poppy IR.

aheejin · 2021-02-08T17:55:28Z

src/passes/Poppify.cpp

+//    (block
+//     (i32.const 42)
+//     (i32.const 5)
+//     (i32.add
+//      (i32.pop)
+//      (i32.pop)
+//     )
+//    )


Suggested change

// (block

// (i32.const 42)

// (i32.const 5)

// (i32.add

// (i32.pop)

// (i32.pop)

// )

// )

// (i32.const 42)

// (i32.const 5)

// (i32.add

// (pop i32)

// (pop i32)

// )

From looking at your test cases, we don't seem to add a new block?

pop printing format fix

We do generate the block, but then the normal printing logic that elides the blocks that make up function bodies kicks in so the block is not printed. You know it's there because there are multiple instructions in the function body, though. For the purposes of this documentation, I think it's important to show that the block is there even if it won't be explicitly printed.

Yeah but that block, which is not printed, exists before the Poppify pass, and your comment looks like Poppify generates it because it doesn't show in the 'before' example but it does in the 'after' example. I think it'd be consistent to show it in both or show it in neither.

In this case because the function body is just a single i32.add expression, there won't be a block containing the body before the Poppify pass. Poppify really does generate a new block in this case.

aheejin · 2021-02-08T18:17:29Z

src/passes/Poppify.cpp

+  // scope and clear the current scope.
+  void patchInstrs(Expression*& expr);
+
+  // BinaryIRWriter methods


Suggested change

// BinaryIRWriter methods

// BinaryenIRWriter methods

aheejin · 2021-02-08T18:29:57Z

src/passes/Poppify.cpp

+        auto* loop = curr->cast<Loop>();
+        if (loop->name.is()) {
+          // TODO: support loop parameters
+          labelTypes[loop->name] = Type::none;


Where is labelTypes used? Is there any reason we don't assign the loop's type here?

Good catch, it is not. I briefly went through a period where I was recomputing types as I traversed the module, but it's much simpler to just use ReFinalize like any other pass would.

aheejin · 2021-02-08T20:07:05Z

test/lit/passes/poppify.wast

@@ -0,0 +1,411 @@
+;; NOTE: Assertions have been generated by update_lit_checks.py and should not be edited.


It would be good to add some tests that contains more nested blocks as expressions, such as

(func $nested (result i32) (block (result i32) (i32.add (block (result i32) (i32.const 3) ) (i32.const 0) ) ) )

aheejin · 2021-02-08T21:02:47Z

src/passes/Poppify.cpp

+  void run(PassRunner* runner, Module* module) {
+    PassRunner subRunner(runner);
+    subRunner.add(std::make_unique<PoppifyFunctionsPass>());
+    subRunner.add(std::make_unique<ReFinalize>());


Why do we need refinalization?

Consider this Binaryen IR block:

(block $l i32 (br $l (unreachable) (i32.const 0) )

In Binaryen IR, this block would have to have type i32 because it ends in an i32.const. However, the BinaryenIRWriter would not emit anything after the unreachable, so in Poppy IR we would end up with:

(block $l i32 (unreachable) )

We want to refinalize this block to give it type unreachable because there is no reason to keep it typed as i32. It's better to give additional optimizations the flexibility and power to do things to this block that do not preserve its i32 type.

aheejin · 2021-02-08T21:42:18Z

src/passes/Poppify.cpp

+
+// Generate names for the elements of tuple globals
+Name getGlobalElem(Name global, Index i) {
+  return Name(std::string(global.c_str()) + '$' + std::to_string(i));


What if the original module already has two globals such as myglobal and myglobal$1?

Good point, this code will do the wrong thing in that case. I will make this more robust.

aheejin · 2021-02-08T21:44:22Z

src/passes/Poppify.cpp

+  auto& instrs = scopeStack.back().instrs;
+  if (curr->value->type.isTuple()) {
+    auto types = module->getGlobal(curr->name)->type;
+    for (Index i = types.size(); i > 0; --i) {


Nit: Do we need to iterate in the reverse order? (emitLocalSet does though)

Yes. Consider this:

(global.set $tuple (tuple.make (i32.const 0) (i32.const 1) (i32.const 2) ) )

The tuple.make is eliminated by Poppification, so we end up with 0, 1, and 2 on the stack. Since the 2 is at the top of stack, it needs to be consumed first, so we need to work through the values from last pushed to first pushed.

aheejin · 2021-02-08T21:48:37Z

src/passes/Poppify.cpp

+    std::vector<std::unique_ptr<Global>> newGlobals;
+    for (int g = module->globals.size() - 1; g >= 0; --g) {
+      const auto& global = *module->globals[g];
+      if (global.type.isTuple()) {


Nit: Maybe we can make this an early exit in case it is not a tuple, given that this loop has also other for and ifs below

aheejin · 2021-02-08T21:53:44Z

test/lit/passes/poppify.wast

+  )
+
+  ;; CHECK:      (func $local-get-tuple (result i32 i64)
+  ;; CHECK-NEXT:  (local $x (i32 i64))


When splitting a global, we seem to remove the original tuple global. Would it be good to do that for locals too? (Not necessarily in this PR though)

I plan to use existing passes like CoalesceLocals to do that as a follow-up optimization. I guess in principle we could use RemoveUnusedModuleElements to clean up the globals as well, but that's simple enough to do here.

aheejin

I posted a few remaining questions but they are mostly minor. LGTM!

tlively requested review from kripken and aheejin February 4, 2021 00:08

tlively mentioned this pull request Feb 4, 2021

StackSignature LUBs #3543

Merged

kripken reviewed Feb 4, 2021

View reviewed changes

tlively force-pushed the stackify-for-real-this-time branch from 8f134d8 to fe2b3af Compare February 4, 2021 07:25

tlively changed the title ~~Stackify pass~~ Poppify pass Feb 4, 2021

tlively requested a review from kripken February 4, 2021 20:57

kripken reviewed Feb 4, 2021

View reviewed changes

aheejin reviewed Feb 8, 2021

View reviewed changes

tlively added 2 commits February 8, 2021 17:12

Fix pop formatting in comment

01af669

Address feedback

b19e01b

aheejin approved these changes Feb 9, 2021

View reviewed changes

tlively added 2 commits February 9, 2021 12:13

Update comments and catch test

708ae89

patchInstrs => patchScope and pop scope

a2627d1

tlively merged commit 9752196 into WebAssembly:main Feb 9, 2021

tlively deleted the stackify-for-real-this-time branch February 9, 2021 22:41

tlively mentioned this pull request Mar 14, 2022

Replacing Stack IR with Poppy IR #3059

Open

12 tasks

		@@ -0,0 +1,497 @@
		/*
		* Copyright 2020 WebAssembly Community Group participants

	* Copyright 2020 WebAssembly Community Group participants
	* Copyright 2021 WebAssembly Community Group participants

	// sequence of instructions in raw WebAssembly. Also note that Poppy IR's
	// sequence of instructions in a WebAssembly binary. Also note that Poppy IR's

	// block unreahchability rules.
	// block unreachability rules.

		@@ -0,0 +1,411 @@
		;; NOTE: Assertions have been generated by update_lit_checks.py and should not be edited.

Poppify pass #3541

Poppify pass #3541

Conversation

tlively commented Feb 4, 2021 • edited Loading

kripken left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tlively commented Feb 4, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aheejin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aheejin left a comment

Choose a reason for hiding this comment

tlively commented Feb 4, 2021 •

edited

Loading