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

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`.

Author: aheejin

CHANGELOG.md

@@ -15,6 +15,8 @@ full changeset diff at the end of each section.
 Current Trunk
 -------------
 
+- `local.tee`'s C/Binaryen.js API now takes an additional type parameter for its
+  local type, like `local.get`. This is required to handle subtypes.
 - Added load_splat SIMD instructions
 - Binaryen.js instruction API changes:
   - `notify` -> `atomic.notify`

src/asm2wasm.h

@@ -1907,7 +1907,7 @@ Function* Asm2WasmBuilder::processFunction(Ref ast) {
         auto ret = allocator.alloc<LocalSet>();
         ret->index = function->getLocalIndex(assign->target());
         ret->value = process(assign->value());
-        ret->setTee(false);
+        ret->makeSet();
         ret->finalize();
         return ret;
       }
@@ -2158,7 +2158,7 @@ Function* Asm2WasmBuilder::processFunction(Ref ast) {
             auto set = allocator.alloc<LocalSet>();
             set->index = function->getLocalIndex(I32_TEMP);
             set->value = value;
-            set->setTee(false);
+            set->makeSet();
             set->finalize();
             auto get = [&]() {
               auto ret = allocator.alloc<LocalGet>();
@@ -2264,7 +2264,7 @@ Function* Asm2WasmBuilder::processFunction(Ref ast) {
                 view.bytes,
                 0,
                 processUnshifted(ast[2][1], view.bytes),
-                builder.makeLocalTee(temp, process(ast[2][2])),
+                builder.makeLocalTee(temp, process(ast[2][2]), type),
                 type),
               builder.makeLocalGet(temp, type));
           } else if (name == Atomics_exchange) {

src/binaryen-c.cpp

@@ -1197,22 +1197,23 @@ BinaryenExpressionRef BinaryenLocalSet(BinaryenModuleRef module,
 
   ret->index = index;
   ret->value = (Expression*)value;
-  ret->setTee(false);
+  ret->makeSet();
   ret->finalize();
   return static_cast<Expression*>(ret);
 }
 BinaryenExpressionRef BinaryenLocalTee(BinaryenModuleRef module,
                                        BinaryenIndex index,
-                                       BinaryenExpressionRef value) {
+                                       BinaryenExpressionRef value,
+                                       BinaryenType type) {
   auto* ret = ((Module*)module)->allocator.alloc<LocalSet>();
 
   if (tracing) {
-    traceExpression(ret, "BinaryenLocalTee", index, value);
+    traceExpression(ret, "BinaryenLocalTee", index, value, type);
   }
 
   ret->index = index;
   ret->value = (Expression*)value;
-  ret->setTee(true);
+  ret->makeTee(Type(type));
   ret->finalize();
   return static_cast<Expression*>(ret);
 }

src/binaryen-c.h

@@ -650,8 +650,10 @@ BINARYEN_API BinaryenExpressionRef BinaryenLocalGet(BinaryenModuleRef module,
                                                     BinaryenType type);
 BINARYEN_API BinaryenExpressionRef BinaryenLocalSet(
   BinaryenModuleRef module, BinaryenIndex index, BinaryenExpressionRef value);
-BINARYEN_API BinaryenExpressionRef BinaryenLocalTee(
-  BinaryenModuleRef module, BinaryenIndex index, BinaryenExpressionRef value);
+BINARYEN_API BinaryenExpressionRef BinaryenLocalTee(BinaryenModuleRef module,
+                                                    BinaryenIndex index,
+                                                    BinaryenExpressionRef value,
+                                                    BinaryenType type);
 BINARYEN_API BinaryenExpressionRef BinaryenGlobalGet(BinaryenModuleRef module,
                                                      const char* name,
                                                      BinaryenType type);

src/ir/ExpressionManipulator.cpp

@@ -91,7 +91,7 @@ flexibleCopy(Expression* original, Module& wasm, CustomCopier custom) {
     }
     Expression* visitLocalSet(LocalSet* curr) {
       if (curr->isTee()) {
-        return builder.makeLocalTee(curr->index, copy(curr->value));
+        return builder.makeLocalTee(curr->index, copy(curr->value), curr->type);
       } else {
         return builder.makeLocalSet(curr->index, copy(curr->value));
       }

src/ir/localize.h

@@ -36,7 +36,7 @@ struct Localizer {
       index = set->index;
     } else {
       index = Builder::addVar(func, expr->type);
-      expr = Builder(*wasm).makeLocalTee(index, expr);
+      expr = Builder(*wasm).makeLocalTee(index, expr, expr->type);
     }
   }
 };

src/js/binaryen.js-post.js

@@ -540,8 +540,11 @@ function wrapModule(module, self) {
     'set': function(index, value) {
       return Module['_BinaryenLocalSet'](module, index, value);
     },
-    'tee': function(index, value) {
-      return Module['_BinaryenLocalTee'](module, index, value);
+    'tee': function(index, value, type) {
+      if (typeof type === 'undefined') {
+        throw new Error("local.tee's type should be defined");
+      }
+      return Module['_BinaryenLocalTee'](module, index, value, type);
     }
   }
 

src/passes/Flatten.cpp

@@ -172,9 +172,10 @@ struct Flatten
             replaceCurrent(set->value); // trivial, no set happens
           } else {
             // use a set in a prelude + a get
-            set->setTee(false);
+            set->makeSet();
             ourPreludes.push_back(set);
-            replaceCurrent(builder.makeLocalGet(set->index, set->value->type));
+            Type localType = getFunction()->getLocalType(set->index);
+            replaceCurrent(builder.makeLocalGet(set->index, localType));
           }
         }
       } else if (auto* br = curr->dynCast<Break>()) {

src/passes/LocalCSE.cpp

@@ -184,8 +184,9 @@ struct LocalCSE : public WalkerPass<LinearExecutionWalker<LocalCSE>> {
         if (iter != usables.end()) {
           // already exists in the table, this is good to reuse
           auto& info = iter->second;
+          Type localType = getFunction()->getLocalType(info.index);
           set->value =
-            Builder(*getModule()).makeLocalGet(info.index, value->type);
+            Builder(*getModule()).makeLocalGet(info.index, localType);
           anotherPass = true;
         } else {
           // not in table, add this, maybe we can help others later

src/passes/MergeLocals.cpp

@@ -88,7 +88,7 @@ struct MergeLocals
     if (auto* get = curr->value->dynCast<LocalGet>()) {
       if (get->index != curr->index) {
         Builder builder(*getModule());
-        auto* trivial = builder.makeLocalTee(get->index, get);
+        auto* trivial = builder.makeLocalTee(get->index, get, get->type);
         curr->value = trivial;
         copies.push_back(curr);
       }