OptimizeInstructions: Handle trivial ref.cast and ref.test by kripken · Pull Request #4097 · WebAssembly/binaryen

kripken · 2021-08-20T21:58:02Z

If the types are completely incompatible, we know the cast will fail. However,
ref.cast does allow a null to pass through, which makes it a little more
complicated.

aheejin

Haven't finished reading the tests; some questions:

aheejin · 2021-08-21T04:24:21Z

src/passes/OptimizeInstructions.cpp

+        // Avoid a type change by forcing to be non-nullable. In practice, this
+        // would have trapped before we get here, so this is just for


Why would this have trapped before we get here?

We receive a null value from a place that has a non-nullable type. Somehow the null falls through there, which I think is only possible using ref.as_non_null which will trap on a null.

aheejin · 2021-08-21T04:54:27Z

src/passes/OptimizeInstructions.cpp

+      // null, or it will return null if it is (and we have already handled the
+      // case of null before).


This sounds like this can't be null because we've already handled the case, but we've only handed it is ref.null, and this can still be null at runtime (as the comment in line 1335-1336 says). I think this comment is a little confusing.

Good point, yeah, this is unclear. I'll rephrase.

aheejin · 2021-08-21T06:10:39Z

src/ir/reorderer.h

+//
+// we generate something so that it is valid to write
+//
+//  (Reorderer.second, Reorderer.first)


How about calling the new first first and the new second second? Because it is within Reorderer, I feel like it is less confusing for Reorderer's first should be the reordered first. To me it is harder to read the code assuming that this first should go second in the result and vice versa.

Interesting, it's the opposite for me 😄

I think about it as "maybe the Reorderer will do nothing, and so Reorderer.second = second, which means the user wants to put that first when they reorder".

How about (Reorderer.reorderedFirst, Reorderer.reorderedSecond)? But that is still ambiguous...

Hmm, my preference is in the order of first/second > reorderedFirst/reorderedSecond > second/first(the status quo), but if you feel this is more confusing, it is possible that this is just my intuition. I don't feel strongly, so let's leave it then.

aheejin · 2021-08-23T21:20:27Z

src/passes/OptimizeInstructions.cpp

+        Reorderer reorderer(
+          curr->ref, curr->rtt, getFunction(), getModule(), passOptions);


Do we not need to check if they can be reordered effect-wise?

That is what the Reorderer does - it potentially adds more code to handle that problem (it adds a local and saves the value here, keeping their order identical).

aheejin · 2021-08-23T21:23:55Z

test/lit/passes/inlining-optimizing.wast

- ;; CHECK-NEXT:     (ref.func $0)
- ;; CHECK-NEXT:     (rtt.canon $none_=>_i32)
- ;; CHECK-NEXT:    )
+ ;; CHECK-NEXT:    (unreachable)


Do we need to change something in the comment below?

aheejin · 2021-08-23T21:25:45Z

test/lit/passes/optimize-instructions-call_ref.wast

+ ;; CHECK-NEXT:    (drop
+ ;; CHECK-NEXT:     (rtt.canon $none_=>_i32)
+ ;; CHECK-NEXT:    )
+ ;; CHECK-NEXT:    (unreachable)


Here too; does the comment need changing?

Sounds good, thanks, I'm updating these comments too.

aheejin · 2021-08-24T01:56:47Z

src/ir/reorderer.h

+//
+// we generate something so that it is valid to write
+//
+//  (Reorderer.second, Reorderer.first)


Hmm, my preference is in the order of first/second > reorderedFirst/reorderedSecond > second/first(the status quo), but if you feel this is more confusing, it is possible that this is just my intuition. I don't feel strongly, so let's leave it then.

aheejin · 2021-08-24T02:36:40Z

test/lit/passes/optimize-instructions-gc.wast

+  ;; CHECK:      (type $array (array (mut i8)))
+
+  ;; CHECK:      (type $B (struct (field i32) (field i32) (field f32)))
+


I wonder what the reason for this change is? Apparently $array and $B haven't changed...

I think this is just because more uses were added, so it alters the order in which the types are emitted (more frequently used ones appear first).

aheejin · 2021-08-24T02:40:59Z

src/passes/OptimizeInstructions.cpp

      optimizer.walkFunction(func);
    }
+
+    TypeUpdating::handleNonDefaultableLocals(func, *getModule());


Would be helpful to add a comment that Reorderer can create locals so we need this

Good point, adding a comment.

aheejin · 2021-08-24T02:48:24Z

src/passes/OptimizeInstructions.cpp

+      curr->ref, getPassOptions(), getModule()->features);
+
+    // If the value is a null, it will just flow through, and we do not need the
+    // cast. However, if that would change the type, then things are less


If this expression is null and changes type from such as a struct to an array, would that possibly make the parent or grandparent expressions of this not validate? We fix nullability but what if the heaptype itself changes? Can Refinailize solve it?

The code here is actually careful about the heap type, I'll improve the comment though.

Basically, we use the heap type of the RTT to create the null - that is the type on the outside.

aheejin · 2021-08-24T03:09:33Z

src/passes/OptimizeInstructions.cpp

+      // Aside from the issue of type incompatibility as mentioned above, the
+      // cast can trap if the types *are* compatible but it happens to be the
+      // case at runtime that the value is not of the desired subtype. If we
+      // do not consider such traps possible, we can ignore that. Note, though,
+      // that we cannot do this if we cannot replace the current type with the
+      // reference's type.


Are traps possible even if ref's heap type is a subtype of rtt's heap type?

Yes, this is an unfortunate property of rtts. E.g. (rtt.canon $x) != (rtt.sub $x (rtt.canon $x)).

This is bad for optimization, so we are hoping to prototype a more static approach soon, that would be as you describe basically.

(rtt.canon $x) != (rtt.sub $x (rtt.canon $x))

😨

kripken · 2021-08-24T18:48:22Z

I think your intuition about the Reorderer is right, the more I think about it. I rewrote it to a method getResultOfFirst that I think is unambiguous, PTAL.

@kripken

`ref.cast` can be statically removed when the ref's type is a subtype of the intended RTT type and either of `--ignore-implicit-traps` or `--traps-never-happen` is given: https://github.com/WebAssembly/binaryen/blob/083ab9842ec3d4ca278c95e1a33112ae7cd4d9e5/src/passes/OptimizeInstructions.cpp#L1603-L1624 Some more context: WebAssembly#4097 (comment) But this can create a block in which a `pop` is nested, which makes the `catch` invalid. The test in this PR is the same as the example given by @kripken in WebAssembly#4237. This calls the fixup function `EHUtils::handleBlockNestedPops` at the end of the pass to fix this. Also, because this pass creates a lot of blocks in other patterns, I think it is possible there can be other patterns to cause this kind of `pop` nesting.

@kripken

`ref.cast` can be statically removed when the ref's type is a subtype of the intended RTT type and either of `--ignore-implicit-traps` or `--traps-never-happen` is given: https://github.com/WebAssembly/binaryen/blob/083ab9842ec3d4ca278c95e1a33112ae7cd4d9e5/src/passes/OptimizeInstructions.cpp#L1603-L1624 Some more context: WebAssembly#4097 (comment) But this can create a block in which a `pop` is nested, which makes the `catch` invalid. The test in this PR is the same as the example given by @kripken in WebAssembly#4237. This calls the fixup function `EHUtils::handleBlockNestedPops` at the end of the pass to fix this. Also, because this pass creates a lot of blocks in other patterns, I think it is possible there can be other patterns to cause this kind of `pop` nesting.

@kripken

`ref.cast` can be statically removed when the ref's type is a subtype of the intended RTT type and either of `--ignore-implicit-traps` or `--traps-never-happen` is given: https://github.com/WebAssembly/binaryen/blob/083ab9842ec3d4ca278c95e1a33112ae7cd4d9e5/src/passes/OptimizeInstructions.cpp#L1603-L1624 Some more context: #4097 (comment) But this can create a block in which a `pop` is nested, which makes the `catch` invalid. The test in this PR is the same as the example given by @kripken in #4237. This calls the fixup function `EHUtils::handleBlockNestedPops` at the end of the pass to fix this. Also, because this pass creates a lot of blocks in other patterns, I think it is possible there can be other patterns to cause this kind of `pop` nesting.

@kripken

`ref.cast` can be statically removed when the ref's type is a subtype of the intended RTT type and either of `--ignore-implicit-traps` or `--traps-never-happen` is given: https://github.com/WebAssembly/binaryen/blob/083ab9842ec3d4ca278c95e1a33112ae7cd4d9e5/src/passes/OptimizeInstructions.cpp#L1603-L1624 Some more context: WebAssembly#4097 (comment) But this can create a block in which a `pop` is nested, which makes the `catch` invalid. The test in this PR is the same as the example given by @kripken in WebAssembly#4237. This calls the fixup function `EHUtils::handleBlockNestedPops` at the end of the pass to fix this. Also, because this pass creates a lot of blocks in other patterns, I think it is possible there can be other patterns to cause this kind of `pop` nesting.

@kripken

`ref.cast` can be statically removed when the ref's type is a subtype of the intended RTT type and either of `--ignore-implicit-traps` or `--traps-never-happen` is given: https://github.com/WebAssembly/binaryen/blob/083ab9842ec3d4ca278c95e1a33112ae7cd4d9e5/src/passes/OptimizeInstructions.cpp#L1603-L1624 Some more context: WebAssembly#4097 (comment) But this can create a block in which a `pop` is nested, which makes the `catch` invalid. The test in this PR is the same as the example given by @kripken in WebAssembly#4237. This calls the fixup function `EHUtils::handleBlockNestedPops` at the end of the pass to fix this. Also, because this pass creates a lot of blocks in other patterns, I think it is possible there can be other patterns to cause this kind of `pop` nesting.

kripken added 15 commits August 13, 2021 13:49

work

ca36e1f

work

8d6f325

format

12209ef

fix

06e5cec

Merge remote-tracking branch 'origin/main' into badcast

86cdfd1

format

567ba4d

fix

6d9e883

fix

80633ac

fix

0a4bcdf

fix

0629cd4

TODO

7b59bee

test

a933c91

fix

1690f51

test

2a75422

format

32b0f42

kripken requested review from aheejin and tlively August 20, 2021 21:58

kripken added 4 commits August 20, 2021 16:11

fix

740d448

format

360e79e

fix

c0c11ee

fix old comments

7469918

aheejin reviewed Aug 23, 2021

View reviewed changes

kripken added 2 commits August 23, 2021 15:20

feedback

1a583d5

feedback

43a4057

aheejin reviewed Aug 24, 2021

View reviewed changes

kripken added 2 commits August 24, 2021 11:45

feedback

fd57dc4

format

4a38d3d

kripken added 2 commits August 24, 2021 12:41

test update

7ad15fd

test update

f7c94dd

aheejin approved these changes Aug 24, 2021

View reviewed changes

kripken merged commit 0a6de17 into main Aug 24, 2021

kripken deleted the badcast branch August 24, 2021 20:41

aheejin mentioned this pull request Dec 23, 2021

[EH][GC] Fix nested pop after removing ref.cast #4407

Merged

		// Avoid a type change by forcing to be non-nullable. In practice, this
		// would have trapped before we get here, so this is just for

		// null, or it will return null if it is (and we have already handled the
		// case of null before).

		Reorderer reorderer(
		curr->ref, curr->rtt, getFunction(), getModule(), passOptions);

		;; CHECK: (type $array (array (mut i8)))

		;; CHECK: (type $B (struct (field i32) (field i32) (field f32)))

Conversation

kripken commented Aug 20, 2021

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!

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!

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!

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!

kripken commented Aug 24, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants