[Wasm GC] Optimize RefIs and RefAs when the type lets us by kripken · Pull Request #3725 · WebAssembly/binaryen

kripken · 2021-03-24T19:57:48Z

This is similar to the optimization of BrOn in #3719 and #3724. When the
type tells us the kind of input we have, we can tell at compile time what
result we'll get, like ref.is_func of something with type (ref func) will
always return 1, etc.

Those previous PRs optimized things that look obvious in terms of what
Binaryen should be doing, but this PR is a little less clear to me (especially
after @aheejin 's comments in #3724 that made me realize this is less
trivial than it seems), so cc'ing some VM people for thoughts:

@jakobkummerow @manoskouk @lars-t-hansen @eqrion

The main open question is the second TODO in the code, which is
basically: Is ref.is_func as fast as ref.is_null? If the latter is a null
check then maybe not. And what about ref.is_func vs the combination
of ref.is_null + i32.eqz?

aheejin · 2021-03-30T10:04:02Z

+    // since RefIsNull is a single byte (the others are 2), while adding a Const
+    // of 0 would be two bytes. Other factors are that we can remove the input


Does this include drop in the calculation too? For example, in RefIsNull case, we save one byte by removing ref.is_null, add one byte by drop, and add two more bytes for i32.const x, so isn't that net +2 bytes gain? (+1 byte gain for other ref.is_*** instructions)

I should have mentioned the drop too, yes. In most cases the input value has no side effects so both it and the drop for it can be removed.

aheejin · 2021-03-30T10:05:52Z

+      // This is the wrong kind, so it will trap. The binaryen optimizer does
+      // not differentiate traps, so we can perform a replacement here. We
+      // replace 2 bytes of ref.as_* with one byte of unreachable and one of a
+      // drop, and the drop can be optimized out later, so this saves both work


and the drop can be optimized out later

Isn't this the case only when the value does not have a side effect?

Correct, I'll fix the comment.

aheejin

By the way did the VM people respond to your questions on the efficiency of the instructions?

aheejin · 2021-03-30T10:25:17Z

+  ;; CHECK-NEXT:   )
+  ;; CHECK-NEXT:  )
+  ;; CHECK-NEXT: )
+  (func $unneeded_is


Existing tests in this file uses 2-space indent, so better be consistent? (I know 1-space and 2-space indents are mixed in many of our tests, but I guess it'd be good to be consistent for new tests)

Good point, I'll fix the whitespace.

aheejin · 2021-03-30T10:29:00Z

+  ;; CHECK-NEXT:  )
+  ;; CHECK-NEXT: )
+  (func $unneeded_is_bad_kinds
+   (param $func (ref null func))


Suggested change

(param $func (ref null func))

(param $func (ref func))

Make one of the params non-nullable to test both nullable and non-nullable cases?

That is tested lower down, we do ref.as_non_null on the nullable params to get non-nullable inputs. I'll add a comment to make it more obvious to the eye.

aheejin · 2021-03-30T10:29:39Z

+  ;; CHECK-NEXT: )
+  (func $unneeded_as_bad_kinds
+   (param $func (ref null func))
+   (param $data (ref null data))


Suggested change

(param $data (ref null data))

(param $data (ref data))

Make one of the params non-nullable to test both nullable and non-nullable cases?

Good catch. I'll add three more testcases below to check those, and a comment.

askeksa-google · 2021-03-30T16:10:31Z

In TurboFan at least, any outermost i32.eqz in the condition of an if or br_if are folded into the branch condition, so they are essentially free.

kripken · 2021-03-30T18:02:22Z

Thanks @askeksa-google ! Makes sense. And binaryen should remove an i32.eqz in those cases, so any left over would need to be actually executed. What's not obvious is how common that is, I guess.

@aheejin That's the only response so far from people familiar with VMs. People are busy I guess...

If we don't get more detailed responses I still think this is a good direction for now. But we should benchmark and evaluate these more later.

…#3725) This is similar to the optimization of BrOn in WebAssembly#3719 and WebAssembly#3724. When the type tells us the kind of input we have, we can tell at compile time what result we'll get, like ref.is_func of something with type (ref func) will always return 1, etc. There are some code size and perf tradeoffs that should be looked into more and are marked as TODOs.

jakobkummerow · 2021-04-07T16:16:05Z

Is ref.is_func as fast as ref.is_null?

I would assume that ref.is_null is faster, though as always it's difficult to say by how much.

In our current implementation, a null check is a pointer comparison against a known constant (which in turn needs to be loaded or materialized, the cost of which depends on how long it's been since the last null check). A func check involves two chained loads (object -> shape descriptor -> type). In fact, our current implementation of is_func initially performs an is_null check as a fast path; however that's technically not necessary and we should probably measure at some point whether it makes more sense to keep or to drop that fast path.

kripken · 2021-04-07T17:01:07Z

I see, thanks @jakobkummerow - very good to know the details here.

Sounds like this PR is in the right direction then. And we can evaluate more in detail as we get benchmarks.

kripken and others added 27 commits March 23, 2021 16:14

work

6c156fb

fix

aad79d1

format

636b7c5

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

0e1c0b7

work

d81ad89

update tests

89b62e5

format

56facfa

cleanup

a1749ac

missing file

b98c2ab

wip

2f89144

builds

46821e4

work

e76d937

format

d2d65dc

comments

89e22a7

update test

cd59733

another test

2f6aaa0

finish

d429a06

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

39bc8a8

typo

fc97701

comment fix

d864f51

fix

be64106

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

07dead3

fix

87ec257

fix off by one

3812709

update test

d03b706

unreachability

29f466e

unreachability

812ac0c

kripken mentioned this pull request Mar 29, 2021

Error on implicit upcast #3636

Closed

kripken requested review from aheejin and tlively March 29, 2021 19:46

aheejin reviewed Mar 30, 2021

View reviewed changes

kripken added 6 commits March 30, 2021 10:45

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

714f73a

improve comment

765b2a2

improve comment

6205138

fix whitespace

3ba6c57

whitespace

80e9a53

add more tests

1166c2a

aheejin approved these changes Mar 30, 2021

View reviewed changes

kripken merged commit 38c1193 into main Mar 30, 2021

kripken deleted the optrefasis2 branch March 30, 2021 19:32

		// since RefIsNull is a single byte (the others are 2), while adding a Const
		// of 0 would be two bytes. Other factors are that we can remove the input

Conversation

kripken commented Mar 24, 2021

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 left a comment

Choose a reason for hiding this comment

Uh oh!

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!

askeksa-google commented Mar 30, 2021

Uh oh!

kripken commented Mar 30, 2021

Uh oh!

jakobkummerow commented Apr 7, 2021

Uh oh!

kripken commented Apr 7, 2021

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants