Make local.tee's type its local's type #2511

aheejin · 2019-12-06T14:05:35Z

According to the current spec, local.tee's return type should be the
same as its local's type. (Discussions on whether we should change this
rule is going on in WebAssembly/reference-types#55, but here I will
assume this spec does not change. If this changes, we should change many
parts of Binaryen transformation anyway...)

But currently in Binaryen local.tee's type is computed from its
value's type. This didn't make any difference in the MVP, but after we
have subtype relationship in #2451, this can become a problem. For
example:

(func $test (result funcref) (local $0 anyref)
  (local.tee $0
    (ref.func $test)
  )
)

This shouldn't validate in the spec, but this will pass Binaryen
validation with the current local.tee implementation.

This makes local.tee's type computed from the local's type, and makes
LocalSet::makeTee get a type parameter, to which we should pass the
its corresponding local's type. We don't embed the local type in the
class LocalSet because it may increase memory size.

This also fixes the type of local.get to be the local type where
local.get and local.set pair is created from local.tee.

According to the current spec, `local.tee`'s return type should be the same as its local's type. (Discussions on whether we should change this rule is going on in WebAssembly/reference-types#55, but here I will assume this spec does not change. If this changes, we should change many parts of Binaryen transformation anyway...) But currently in Binaryen `local.tee`'s type is computed from its value's type. This didn't make any difference in the MVP, but after we have subtype relationship in WebAssembly#2451, this can become a problem. For example: ``` (func $test (result funcref) (local $0 anyref) (local.tee $0 (ref.func $test) ) ) ``` This shouldn't validate in the spec, but this will pass Binaryen validation with the current `local.tee` implementation. This makes `local.tee`'s type computed from the local's type, and makes `LocalSet::makeTee` get a type parameter, to which we should pass the its corresponding local's type. We don't embed the local type in the class `LocalSet` because it may increase memory size. This also fixes the type of `local.get` to be the local type where `local.get` and `local.set` pair is created from `local.tee`.

src/wasm/wasm-validator.cpp

src/wasm-builder.h

kripken · 2019-12-06T22:13:38Z

I'm a little worried about changes in the spec, though, let's discuss that before landing this.

dcodeIO · 2019-12-07T19:03:44Z

src/js/binaryen.js-post.js

-    'tee': function(index, value) {
-      return Module['_BinaryenLocalTee'](module, index, value);
+    'tee': function(index, value, type) {
+      return Module['_BinaryenLocalTee'](module, index, value, type);


Can we make it so that if type is not specified, the type of value is used - to make this backwards compatible?

That would only work in non-reference types. The purpose of this change is to prevent to use the value type for local.tee's type in the first place because of subtypes... Wouldn't that be easy to be used incorrectly?

Mostly thinking in terms of existing projects that don't use reference types yet, where having to change every single local.tee might be quite some work for no immediate benefit. Perhaps only make if backwards compatible for i32, i64, f32, f64?

I am still inclined to change the signature to be consistent; making the old signature only work for old number types seems a little unnatural and also adds checking overhead to every local.tee call. I don't know how high the cost of changing existing code will be, but doesn't all existing code have to undergo significant changes after #2510 lands anyway..? WDYT? cc @kripken

I would like to make the change as less confusing as possible for people with existing code. It seems like they would not provide the parameter, so it would be coerced from undefined to 0, which is none. Then they'd get a validation error later, which might not be that obvious?

Automatically converting the 0 to the proper type is one option, silently - IIUC @dcodeIO that's what you suggest?

Another option is to give an immediate error, if (type === undefined) throw Error('tee() must receive the type as a fourth parameter (this was an API change));` perhaps? A benefit to doing it that way is people will know to update their code, so it won't keep using the old API forever.

What do you mean by

Automatically converting the 0 to the proper type is one option

?

I meant what @dcodeIO said, at least as I understood it:

Can we make it so that if type is not specified, the type of value is used - to make this backwards compatible?

If type is not provided, use the type of the value instead.

What do you think @dcodeIO? I like the idea of requiring type parameter in all local.tee, and explicitly error out if type is undefined. Allowing untyped API only for the number types doesn't seem to be very consistent. Can we land this?

Sure, go ahead :). Should be fine with other breaking changes already landed.

Co-Authored-By: Thomas Lively <[email protected]>

aheejin · 2019-12-08T08:24:49Z

I'm a little worried about changes in the spec, though, let's discuss that before landing this.

Which part of the spec do you mean? The local.tee spec? (I don't think it has changed in any way yet) Or binaryen.js API?

kripken · 2019-12-10T01:17:38Z

src/js/binaryen.js-post.js

-    'tee': function(index, value) {
-      return Module['_BinaryenLocalTee'](module, index, value);
+    'tee': function(index, value, type) {
+      return Module['_BinaryenLocalTee'](module, index, value, type);


I would like to make the change as less confusing as possible for people with existing code. It seems like they would not provide the parameter, so it would be coerced from undefined to 0, which is none. Then they'd get a validation error later, which might not be that obvious?

Automatically converting the 0 to the proper type is one option, silently - IIUC @dcodeIO that's what you suggest?

Another option is to give an immediate error, if (type === undefined) throw Error('tee() must receive the type as a fourth parameter (this was an API change));` perhaps? A benefit to doing it that way is people will know to update their code, so it won't keep using the old API forever.

aheejin requested review from kripken and tlively December 6, 2019 14:05

binaryen.js fix

28d9600

tlively approved these changes Dec 6, 2019

View reviewed changes

src/wasm/wasm-validator.cpp Outdated Show resolved Hide resolved

src/wasm-builder.h Outdated Show resolved Hide resolved

dcodeIO reviewed Dec 7, 2019

View reviewed changes

Update src/wasm/wasm-validator.cpp

e93ec17

Co-Authored-By: Thomas Lively <[email protected]>

aheejin added 3 commits December 8, 2019 00:25

Remove ret->finalize()

320c313

Add to changelog

2341ab1

Merge branch 'master' into local_tee_type

b4b64ee

kripken approved these changes Dec 10, 2019

View reviewed changes

aheejin added 4 commits December 12, 2019 04:30

Merge branch 'master' into local_tee_type

3644694

Error out if local.tee type is undefined

4ad6110

Binaryen.js test result fix

d9c4973

aaaaarg

94966e2

aheejin force-pushed the local_tee_type branch from 2edbea8 to 94966e2 Compare December 12, 2019 23:41

aheejin added 3 commits December 12, 2019 16:22

Change to trigger CI

556ddf4

Change to trigger CI

a937cae

Extract localType as a variable

7a77d50

aheejin merged commit 89d1cf9 into WebAssembly:master Dec 13, 2019

aheejin deleted the local_tee_type branch December 13, 2019 02:36

TerrorJack mentioned this pull request Dec 13, 2019

Update binaryen tweag/asterius#392

Merged

aheejin mentioned this pull request Mar 19, 2021

Bad interaction with tuples and local.tee using the value type #3704

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make local.tee's type its local's type #2511

Make local.tee's type its local's type #2511

aheejin commented Dec 6, 2019

kripken commented Dec 6, 2019

dcodeIO Dec 7, 2019

aheejin Dec 8, 2019

dcodeIO Dec 8, 2019 •

edited

Loading

aheejin Dec 10, 2019 •

edited

Loading

kripken Dec 10, 2019

aheejin Dec 10, 2019 •

edited

Loading

kripken Dec 10, 2019

aheejin Dec 12, 2019

dcodeIO Dec 12, 2019

aheejin commented Dec 8, 2019

kripken Dec 10, 2019

Make local.tee's type its local's type #2511

Make local.tee's type its local's type #2511

Conversation

aheejin commented Dec 6, 2019

kripken commented Dec 6, 2019

dcodeIO Dec 7, 2019

Choose a reason for hiding this comment

aheejin Dec 8, 2019

Choose a reason for hiding this comment

dcodeIO Dec 8, 2019 • edited Loading

Choose a reason for hiding this comment

aheejin Dec 10, 2019 • edited Loading

Choose a reason for hiding this comment

kripken Dec 10, 2019

Choose a reason for hiding this comment

aheejin Dec 10, 2019 • edited Loading

Choose a reason for hiding this comment

kripken Dec 10, 2019

Choose a reason for hiding this comment

aheejin Dec 12, 2019

Choose a reason for hiding this comment

dcodeIO Dec 12, 2019

Choose a reason for hiding this comment

aheejin commented Dec 8, 2019

kripken Dec 10, 2019

Choose a reason for hiding this comment

dcodeIO Dec 8, 2019 •

edited

Loading

aheejin Dec 10, 2019 •

edited

Loading

aheejin Dec 10, 2019 •

edited

Loading