Poppy IR wast parsing and validation by tlively · Pull Request #3105 · WebAssembly/binaryen

tlively · 2020-09-08T02:44:08Z

Adds an IR Profile to each function so the validator can determine
which validation rules to apply and adds a flag to have the wast
parser set the profile to Poppy for testing purposes. See #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.

kripken · 2020-09-08T17:00:44Z

Is this the same as the PR on your fork from earlier?

If so I wonder if this could have been a PR relative to a branch, which is then redirected? That would preserve comments and remember seen files and so forth.

aheejin · 2020-09-08T17:37:18Z

Is this the same as the PR on your fork from earlier?

If so I wonder if this could have been a PR relative to a branch, which is then redirected? That would preserve comments and remember seen files and so forth.

AFAIK this is possible only if the base branch is in the main repo and not a fork. So it's possible for people who have the write access to the repo. When you create a PR you can make it based on a branch, and you can change that base branch later.

tlively · 2020-09-08T20:02:50Z

Yes, this is the same PR from before, with comments addressed.

I guess I could make branches on the main repo in the future, but that's not my usual workflow :(

kripken · 2020-09-09T16:59:24Z

Is it not possible to have all the branches on your fork as you always do, and one is relative to the other? You can adjust the target branch and I'm hoping that includes that the target branch is on a fork versus the main repo.

aheejin · 2020-09-09T12:26:16Z

src/ir/stack-utils.cpp

+bool mayBeUnreachable(Expression* expr) {
+  if (Properties::isControlFlowStructure(expr)) {
+    return true;
+  }


Can't this use StackSignature and precisely compute whether it is really unreachable or not?

We can use StackSignature (or just look at expr->type) to figure out if it is unreachable, but what we want to do in this function is determine if it is allowed to be unreachable.

aheejin · 2020-09-09T17:15:50Z

src/ir/stack-utils.h

+//     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.


Where do we do this? Poppy IR doesn't seem to have its own Block::finalize() or something and it shares the parser with Binaryen IR, aren't block types in Poppy IR assigned in the same way as that of Binaryen IR?

Eventually we will need to add alternate logic to Block::finalize() to handle Poppy IR. The reason I haven't done this yet is because the parser only calls Block::finalize(Type), so none of the tests call Block::finalize(), so we would have no test coverage. The next PR will add the stackification pass, which will call refinalize, so I will implement this with test coverage then.

For example,

(block (result i32) (unreachable) )

According to your explanation, this block's type will be i32 in Binaryen IR and unreachable in Poppy IR. This block has a result type so Block::finalize(type) will be called. But the current Block::finalize(type) respects the type given as the argument and does not check instructions inside for unreachables, no?

What I meant was, as you said, given that we only use Block::finalize(type) for now, how we determine the unreachability of a block will be the same for Binaryen IR and Poppy IR, so this comments block does not seem to match the current implementation. I think I might be missing something.

In both Binaryen IR and Poppy IR, the block in your example can have either i32 or unreachable as its type. In Binaryen IR, the parser will give it i32 type and subsequent optimizations will change it to have unreachable type. In Poppy IR, the parser will similarly give it i32 type, but we don't yet have any code that will change the type to unreachable (even though that would be allowed).

The difference between Poppy IR and Binaryen IR is in a block like this:

(block (unreachable) (i32.const 0) )

In Binaryen IR, it would be illegal for that block to have unreachable type, but in Poppy IR it could still have either unreachable or i32 type.

I see, thanks. Is there any particular reason for this distinction?

Yes, it has to do with type inference in Poppy IR, which we don't do in Binaryen IR. Consider the following function:

(func (result i32) (block $body (unreachable) (i32.const 0) ) )

Binaryen IR unconditionally gives unreachable blocks none type when writing them out, so if $body were allowed to be unreachable in Binaryen IR, there resulting binary would be invalid. In contrast, Poppy IR does type inference before emitting binaries, which correctly gives $body an i32 type.

aheejin · 2020-09-09T17:19:01Z

Is it not possible to have all the branches on your fork as you always do, and one is relative to the other? You can adjust the target branch and I'm hoping that includes that the target branch is on a fork versus the main repo.

No you can't make a branch in your fork as a base branch. Not sure why Github made that restriction. Anyway I don't mind you pre-opening PRs in your fork though.

tlively · 2020-09-09T17:25:23Z

Is it not possible to have all the branches on your fork as you always do, and one is relative to the other? You can adjust the target branch and I'm hoping that includes that the target branch is on a fork versus the main repo.

No, I just verified that this is not possible. I can retarget the PR, but only to branches on the same fork of the repo. This makes sense because retargeting a PR to a different fork would essentially move it to that fork and assign it a new number, so the PR would lose its identity.

Edit: Just saw that Heejin tried the same thing.

aheejin

LGTM!

aheejin · 2020-09-09T19:33:44Z

src/ir/stack-utils.h

+//     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.


I see, thanks. Is there any particular reason for this distinction?

kripken · 2020-09-09T21:08:08Z

src/ir/stack-utils.h


+// Whether `expr` may be unreachable in Poppy IR. True for control flow
+// structures and polymorphic unreachable instructions.
+bool mayBeUnreachable(Expression* expr);


I still think my comment about this is relevant: tlively#1 (comment)

The comment here doesn't clearly say which of the two cases it is IMO. If it is "not exited normally" or such, that might be good to clarify?

I mean, is "unreachable" here referring to a type in the type system, or to a particular property, etc.

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 requested review from aheejin and kripken September 8, 2020 02:44

tlively mentioned this pull request Sep 8, 2020

Replacing Stack IR with Poppy IR #3059

Open

12 tasks

aheejin reviewed Sep 9, 2020

View reviewed changes

aheejin approved these changes Sep 9, 2020

View reviewed changes

tlively merged commit 2aa0cf3 into WebAssembly:master Sep 9, 2020

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

kripken reviewed Sep 9, 2020

View reviewed changes

tlively added the Poppy IR label Sep 14, 2020

Conversation

tlively commented Sep 8, 2020

Uh oh!

kripken commented Sep 8, 2020

Uh oh!

aheejin commented Sep 8, 2020

Uh oh!

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

Uh oh!

Uh oh!

kripken commented Sep 9, 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!

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

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

aheejin left a comment

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!

3 participants

tlively commented Sep 8, 2020 •

edited

Loading

aheejin commented Sep 9, 2020 •

edited

Loading

tlively commented Sep 9, 2020 •

edited

Loading