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

aheejin · web-flow · commit 89d1cf92be06 · 2019-12-12T18:36:31.000-08:00
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`.
diff --git a/CHANGELOG.md b/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`
diff --git a/src/asm2wasm.h b/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) {
diff --git a/src/binaryen-c.cpp b/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);
 }
diff --git a/src/binaryen-c.h b/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);
diff --git a/src/ir/ExpressionManipulator.cpp b/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));
       }
diff --git a/src/ir/localize.h b/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);
     }
   }
 };
diff --git a/src/js/binaryen.js-post.js b/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);
     }
   }
 
diff --git a/src/passes/Flatten.cpp b/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>()) {
diff --git a/src/passes/LocalCSE.cpp b/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
diff --git a/src/passes/MergeLocals.cpp b/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);
       }
diff --git a/src/passes/RemoveUnusedBrs.cpp b/src/passes/RemoveUnusedBrs.cpp
@@ -287,7 +287,7 @@ struct RemoveUnusedBrs : public WalkerPass<PostWalker<RemoveUnusedBrs>> {
         Expression* z;
         replaceCurrent(
           z = builder.makeIf(
-            builder.makeLocalTee(temp, curr->condition),
+            builder.makeLocalTee(temp, curr->condition, i32),
             builder.makeIf(builder.makeBinary(EqInt32,
                                               builder.makeLocalGet(temp, i32),
                                               builder.makeConst(Literal(int32_t(
@@ -1074,7 +1074,7 @@ struct RemoveUnusedBrs : public WalkerPass<PostWalker<RemoveUnusedBrs>> {
         iff->finalize();
         Expression* replacement = iff;
         if (tee) {
-          set->setTee(false);
+          set->makeSet();
           // We need a block too.
           replacement = builder.makeSequence(iff,
                                              get // reuse the get
diff --git a/src/passes/SSAify.cpp b/src/passes/SSAify.cpp
@@ -154,7 +154,7 @@ struct SSAify : public Pass {
         if (set) {
           // a set exists, just add a tee of its value
           auto* value = set->value;
-          auto* tee = builder.makeLocalTee(new_, value);
+          auto* tee = builder.makeLocalTee(new_, value, get->type);
           set->value = tee;
           // the value may have been something we tracked the location
           // of. if so, update that, since we moved it into the tee
diff --git a/src/passes/SimplifyLocals.cpp b/src/passes/SimplifyLocals.cpp
@@ -256,7 +256,7 @@ struct SimplifyLocals
       } else {
         this->replaceCurrent(set);
         assert(!set->isTee());
-        set->setTee(true);
+        set->makeTee(this->getFunction()->getLocalType(set->index));
       }
       // reuse the local.get that is dying
       *found->second.item = curr;
@@ -271,7 +271,7 @@ struct SimplifyLocals
     auto* set = curr->value->dynCast<LocalSet>();
     if (set) {
       assert(set->isTee());
-      set->setTee(false);
+      set->makeSet();
       this->replaceCurrent(set);
     }
   }
@@ -559,7 +559,7 @@ struct SimplifyLocals
       auto* set = (*breakLocalSetPointer)->template cast<LocalSet>();
       if (br->condition) {
         br->value = set;
-        set->setTee(true);
+        set->makeTee(this->getFunction()->getLocalType(set->index));
         *breakLocalSetPointer =
           this->getModule()->allocator.template alloc<Nop>();
         // in addition, as this is a conditional br that now has a value, it now
@@ -728,7 +728,8 @@ struct SimplifyLocals
     ifTrueBlock->finalize();
     assert(ifTrueBlock->type != none);
     // Update the ifFalse side.
-    iff->ifFalse = builder.makeLocalGet(set->index, set->value->type);
+    iff->ifFalse = builder.makeLocalGet(
+      set->index, this->getFunction()->getLocalType(set->index));
     iff->finalize(); // update type
     // Update the get count.
     getCounter.num[set->index]++;
diff --git a/src/passes/Untee.cpp b/src/passes/Untee.cpp
@@ -41,9 +41,10 @@ struct Untee : public WalkerPass<PostWalker<Untee>> {
       } else {
         // a normal tee. replace with set and get
         Builder builder(*getModule());
-        replaceCurrent(builder.makeSequence(
-          curr, builder.makeLocalGet(curr->index, curr->value->type)));
-        curr->setTee(false);
+        LocalGet* get = builder.makeLocalGet(
+          curr->index, getFunction()->getLocalType(curr->index));
+        replaceCurrent(builder.makeSequence(curr, get));
+        curr->makeSet();
       }
     }
   }
diff --git a/src/passes/Vacuum.cpp b/src/passes/Vacuum.cpp
@@ -346,7 +346,7 @@ struct Vacuum : public WalkerPass<ExpressionStackWalker<Vacuum>> {
     // a drop of a tee is a set
     if (auto* set = curr->value->dynCast<LocalSet>()) {
       assert(set->isTee());
-      set->setTee(false);
+      set->makeSet();
       replaceCurrent(set);
       return;
     }
diff --git a/src/tools/fuzzing.h b/src/tools/fuzzing.h
@@ -1277,7 +1277,7 @@ class TranslateToFuzzReader {
     }
     auto* value = make(valueType);
     if (tee) {
-      return builder.makeLocalTee(pick(locals), value);
+      return builder.makeLocalTee(pick(locals), value, valueType);
     } else {
       return builder.makeLocalSet(pick(locals), value);
     }
diff --git a/src/wasm-builder.h b/src/wasm-builder.h
@@ -232,14 +232,15 @@ class Builder {
     auto* ret = allocator.alloc<LocalSet>();
     ret->index = index;
     ret->value = value;
+    ret->makeSet();
     ret->finalize();
     return ret;
   }
-  LocalSet* makeLocalTee(Index index, Expression* value) {
+  LocalSet* makeLocalTee(Index index, Expression* value, Type type) {
     auto* ret = allocator.alloc<LocalSet>();
     ret->index = index;
     ret->value = value;
-    ret->setTee(true);
+    ret->makeTee(type);
     return ret;
   }
   GlobalGet* makeGlobalGet(Name name, Type type) {
diff --git a/src/wasm.h b/src/wasm.h
@@ -709,8 +709,9 @@ class LocalSet : public SpecificExpression<Expression::LocalSetId> {
   Index index;
   Expression* value;
 
-  bool isTee();
-  void setTee(bool is);
+  bool isTee() const;
+  void makeTee(Type type);
+  void makeSet();
 };
 
 class GlobalGet : public SpecificExpression<Expression::GlobalGetId> {
diff --git a/src/wasm/wasm-binary.cpp b/src/wasm/wasm-binary.cpp
@@ -2459,8 +2459,11 @@ void WasmBinaryBuilder::visitLocalSet(LocalSet* curr, uint8_t code) {
     throwError("bad local.set index");
   }
   curr->value = popNonVoidExpression();
-  curr->type = curr->value->type;
-  curr->setTee(code == BinaryConsts::LocalTee);
+  if (code == BinaryConsts::LocalTee) {
+    curr->makeTee(currFunction->getLocalType(curr->index));
+  } else {
+    curr->makeSet();
+  }
   curr->finalize();
 }
 
diff --git a/src/wasm/wasm-emscripten.cpp b/src/wasm/wasm-emscripten.cpp
@@ -112,7 +112,7 @@ inline Expression* stackBoundsCheck(Builder& builder,
   auto check =
     builder.makeIf(builder.makeBinary(
                      BinaryOp::LtUInt32,
-                     builder.makeLocalTee(newSP, value),
+                     builder.makeLocalTee(newSP, value, stackPointer->type),
                      builder.makeGlobalGet(stackLimit->name, stackLimit->type)),
                    builder.makeCall(handler, {}, none));
   // (global.set $__stack_pointer (local.get $newSP))
@@ -172,7 +172,7 @@ void EmscriptenGlueGenerator::generateStackAllocFunction() {
   const static uint32_t bitMask = bitAlignment - 1;
   Const* subConst = builder.makeConst(Literal(~bitMask));
   Binary* maskedSub = builder.makeBinary(AndInt32, sub, subConst);
-  LocalSet* teeStackLocal = builder.makeLocalTee(1, maskedSub);
+  LocalSet* teeStackLocal = builder.makeLocalTee(1, maskedSub, i32);
   Expression* storeStack = generateStoreStackPointer(function, teeStackLocal);
 
   Block* block = builder.makeBlock();
diff --git a/src/wasm/wasm-s-parser.cpp b/src/wasm/wasm-s-parser.cpp
@@ -997,7 +997,7 @@ Expression* SExpressionWasmBuilder::makeLocalTee(Element& s) {
   auto ret = allocator.alloc<LocalSet>();
   ret->index = getLocalIndex(*s[1]);
   ret->value = parseExpression(s[2]);
-  ret->setTee(true);
+  ret->makeTee(currFunction->getLocalType(ret->index));
   ret->finalize();
   return ret;
 }
@@ -1006,7 +1006,7 @@ Expression* SExpressionWasmBuilder::makeLocalSet(Element& s) {
   auto ret = allocator.alloc<LocalSet>();
   ret->index = getLocalIndex(*s[1]);
   ret->value = parseExpression(s[2]);
-  ret->setTee(false);
+  ret->makeSet();
   ret->finalize();
   return ret;
 }
diff --git a/src/wasm/wasm-validator.cpp b/src/wasm/wasm-validator.cpp
@@ -718,15 +718,15 @@ void FunctionValidator::visitLocalSet(LocalSet* curr) {
                    "local.set index must be small enough")) {
     if (curr->value->type != unreachable) {
       if (curr->type != none) { // tee is ok anyhow
-        shouldBeEqualOrFirstIsUnreachable(curr->value->type,
-                                          curr->type,
-                                          curr,
-                                          "local.set type must be correct");
+        shouldBeEqual(getFunction()->getLocalType(curr->index),
+                      curr->type,
+                      curr,
+                      "local.set type must be correct");
       }
-      shouldBeEqual(getFunction()->getLocalType(curr->index),
-                    curr->value->type,
+      shouldBeEqual(curr->value->type,
+                    getFunction()->getLocalType(curr->index),
                     curr,
-                    "local.set type must match function");
+                    "local.set's value type must be correct");
     }
   }
 }
diff --git a/src/wasm/wasm.cpp b/src/wasm/wasm.cpp
@@ -442,24 +442,23 @@ void CallIndirect::finalize() {
   }
 }
 
-bool LocalSet::isTee() { return type != none; }
+bool LocalSet::isTee() const { return type != none; }
 
-void LocalSet::setTee(bool is) {
-  if (is) {
-    type = value->type;
-  } else {
-    type = none;
-  }
+// Changes to local.tee. The type of the local should be given.
+void LocalSet::makeTee(Type type_) {
+  type = type_;
+  finalize(); // type may need to be unreachable
+}
+
+// Changes to local.set.
+void LocalSet::makeSet() {
+  type = none;
   finalize(); // type may need to be unreachable
 }
 
 void LocalSet::finalize() {
   if (value->type == unreachable) {
     type = unreachable;
-  } else if (isTee()) {
-    type = value->type;
-  } else {
-    type = none;
   }
 }
 
diff --git a/test/binaryen.js/kitchen-sink.js b/test/binaryen.js/kitchen-sink.js
@@ -469,7 +469,7 @@ function test_core() {
     ),
     module.drop(module.local.get(0, Binaryen.i32)),
     module.local.set(0, makeInt32(101)),
-    module.drop(module.local.tee(0, makeInt32(102))),
+    module.drop(module.local.tee(0, makeInt32(102), Binaryen.i32)),
     module.i32.load(0, 0, makeInt32(1)),
     module.i64.load16_s(2, 1, makeInt32(8)),
     module.f32.load(0, 0, makeInt32(2)),
diff --git a/test/binaryen.js/kitchen-sink.js.txt b/test/binaryen.js/kitchen-sink.js.txt
diff --git a/test/example/c-api-kitchen-sink.c b/test/example/c-api-kitchen-sink.c
diff --git a/test/example/c-api-kitchen-sink.txt b/test/example/c-api-kitchen-sink.txt
diff --git a/test/example/c-api-relooper-unreachable-if.cpp b/test/example/c-api-relooper-unreachable-if.cpp

Original file line number	Diff line number	Diff line change
`@@ -91,7 +91,7 @@ flexibleCopy(Expression* original, Module& wasm, CustomCopier custom) {`
`91`	`91`	`}`
`92`	`92`	`Expression* visitLocalSet(LocalSet* curr) {`
`93`	`93`	`if (curr->isTee()) {`
`94`		`- return builder.makeLocalTee(curr->index, copy(curr->value));`
	`94`	`+ return builder.makeLocalTee(curr->index, copy(curr->value), curr->type);`
`95`	`95`	`} else {`
`96`	`96`	`return builder.makeLocalSet(curr->index, copy(curr->value));`
`97`	`97`	`}`
Original file line number	Diff line number	Diff line change
`@@ -36,7 +36,7 @@ struct Localizer {`
`36`	`36`	`index = set->index;`
`37`	`37`	`} else {`
`38`	`38`	`index = Builder::addVar(func, expr->type);`
`39`		`- expr = Builder(*wasm).makeLocalTee(index, expr);`
	`39`	`+ expr = Builder(*wasm).makeLocalTee(index, expr, expr->type);`
`40`	`40`	`}`
`41`	`41`	`}`
`42`	`42`	`};`
Original file line number	Diff line number	Diff line change
`@@ -540,8 +540,11 @@ function wrapModule(module, self) {`
`540`	`540`	`'set': function(index, value) {`
`541`	`541`	`return Module['_BinaryenLocalSet'](module, index, value);`
`542`	`542`	`},`
`543`		`- 'tee': function(index, value) {`
`544`		`- return Module['_BinaryenLocalTee'](module, index, value);`
	`543`	`+ 'tee': function(index, value, type) {`
	`544`	`+ if (typeof type === 'undefined') {`
	`545`	`+ throw new Error("local.tee's type should be defined");`
	`546`	`+ }`
	`547`	`+ return Module['_BinaryenLocalTee'](module, index, value, type);`
`545`	`548`	`}`
`546`	`549`	`}`
`547`	`550`
Original file line number	Diff line number	Diff line change
`@@ -172,9 +172,10 @@ struct Flatten`
`172`	`172`	`replaceCurrent(set->value); // trivial, no set happens`
`173`	`173`	`} else {`
`174`	`174`	`// use a set in a prelude + a get`
`175`		`- set->setTee(false);`
	`175`	`+ set->makeSet();`
`176`	`176`	`ourPreludes.push_back(set);`
`177`		`- replaceCurrent(builder.makeLocalGet(set->index, set->value->type));`
	`177`	`+ Type localType = getFunction()->getLocalType(set->index);`
	`178`	`+ replaceCurrent(builder.makeLocalGet(set->index, localType));`
`178`	`179`	`}`
`179`	`180`	`}`
`180`	`181`	`} else if (auto* br = curr->dynCast<Break>()) {`
Original file line number	Diff line number	Diff line change
`@@ -88,7 +88,7 @@ struct MergeLocals`
`88`	`88`	`if (auto* get = curr->value->dynCast<LocalGet>()) {`
`89`	`89`	`if (get->index != curr->index) {`
`90`	`90`	`Builder builder(*getModule());`
`91`		`- auto* trivial = builder.makeLocalTee(get->index, get);`
	`91`	`+ auto* trivial = builder.makeLocalTee(get->index, get, get->type);`
`92`	`92`	`curr->value = trivial;`
`93`	`93`	`copies.push_back(curr);`
`94`	`94`	`}`