Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 13 commits into from
Dec 13, 2019
6 changes: 3 additions & 3 deletions src/asm2wasm.h
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
Expand Down Expand Up @@ -2160,7 +2160,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>();
Expand Down Expand Up @@ -2266,7 +2266,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) {
Expand Down
9 changes: 5 additions & 4 deletions src/binaryen-c.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1261,22 +1261,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);
}
Expand Down
6 changes: 4 additions & 2 deletions src/binaryen-c.h
Original file line number Diff line number Diff line change
Expand Up @@ -665,8 +665,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);
Expand Down
2 changes: 1 addition & 1 deletion src/ir/ExpressionManipulator.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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));
}
Expand Down
2 changes: 1 addition & 1 deletion src/ir/localize.h
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
}
};
Expand Down
4 changes: 2 additions & 2 deletions src/js/binaryen.js-post.js
Original file line number Diff line number Diff line change
Expand Up @@ -541,8 +541,8 @@ 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) {
return Module['_BinaryenLocalTee'](module, index, value, type);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

@dcodeIO dcodeIO Dec 8, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

@aheejin aheejin Dec 10, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

@aheejin aheejin Dec 10, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you mean by

Automatically converting the 0 to the proper type is one option

?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}
}

Expand Down
5 changes: 3 additions & 2 deletions src/passes/Flatten.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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));
replaceCurrent(builder.makeLocalGet(
set->index, getFunction()->getLocalType(set->index)));
}
}
} else if (auto* br = curr->dynCast<Break>()) {
Expand Down
3 changes: 2 additions & 1 deletion src/passes/LocalCSE.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
2 changes: 1 addition & 1 deletion src/passes/MergeLocals.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
Expand Down
4 changes: 2 additions & 2 deletions src/passes/RemoveUnusedBrs.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand Down Expand Up @@ -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
Expand Down
2 changes: 1 addition & 1 deletion src/passes/SSAify.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
9 changes: 5 additions & 4 deletions src/passes/SimplifyLocals.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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);
}
}
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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]++;
Expand Down
7 changes: 4 additions & 3 deletions src/passes/Untee.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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();
}
}
}
Expand Down
2 changes: 1 addition & 1 deletion src/passes/Vacuum.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
Expand Down
2 changes: 1 addition & 1 deletion src/tools/fuzzing.h
Original file line number Diff line number Diff line change
Expand Up @@ -1282,7 +1282,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);
}
Expand Down
6 changes: 4 additions & 2 deletions src/wasm-builder.h
Original file line number Diff line number Diff line change
Expand Up @@ -241,14 +241,16 @@ 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);
ret->finalize();
return ret;
}
GlobalGet* makeGlobalGet(Name name, Type type) {
Expand Down
5 changes: 3 additions & 2 deletions src/wasm.h
Original file line number Diff line number Diff line change
Expand Up @@ -725,8 +725,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> {
Expand Down
7 changes: 5 additions & 2 deletions src/wasm/wasm-binary.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -2527,8 +2527,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();
}

Expand Down
4 changes: 2 additions & 2 deletions src/wasm/wasm-emscripten.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -113,7 +113,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))
Expand Down Expand Up @@ -173,7 +173,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();
Expand Down
4 changes: 2 additions & 2 deletions src/wasm/wasm-s-parser.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1040,7 +1040,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;
}
Expand All @@ -1049,7 +1049,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;
}
Expand Down
8 changes: 7 additions & 1 deletion src/wasm/wasm-validator.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -720,7 +720,13 @@ void FunctionValidator::visitLocalSet(LocalSet* curr) {
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");
curr->value->type,
curr->type,
curr,
"local.set's value type must be correct");
shouldBeTrue(curr->type == getFunction()->getLocalType(curr->index),
curr,
"local.set's tpye must be correct");
}
shouldBeEqual(getFunction()->getLocalType(curr->index),
curr->value->type,
Expand Down
21 changes: 10 additions & 11 deletions src/wasm/wasm.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -469,24 +469,23 @@ bool FunctionType::operator==(FunctionType& b) {
}
bool FunctionType::operator!=(FunctionType& b) { return !(*this == b); }

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;
}
}

Expand Down
2 changes: 1 addition & 1 deletion test/binaryen.js/kitchen-sink.js
Original file line number Diff line number Diff line change
Expand Up @@ -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)),
Expand Down
2 changes: 1 addition & 1 deletion test/binaryen.js/kitchen-sink.js.txt
Original file line number Diff line number Diff line change
Expand Up @@ -5545,7 +5545,7 @@ int main() {
expressions[736] = BinaryenConst(the_module, BinaryenLiteralInt32(101));
expressions[737] = BinaryenLocalSet(the_module, 0, expressions[736]);
expressions[738] = BinaryenConst(the_module, BinaryenLiteralInt32(102));
expressions[739] = BinaryenLocalTee(the_module, 0, expressions[738]);
expressions[739] = BinaryenLocalTee(the_module, 0, expressions[738], 2);
expressions[740] = BinaryenDrop(the_module, expressions[739]);
expressions[741] = BinaryenConst(the_module, BinaryenLiteralInt32(1));
expressions[742] = BinaryenLoad(the_module, 4, 1, 0, 0, 2, expressions[741]);
Expand Down
4 changes: 3 additions & 1 deletion test/example/c-api-kitchen-sink.c
Original file line number Diff line number Diff line change
Expand Up @@ -642,7 +642,9 @@ void test_core() {
module, makeInt32(module, 2449), callOperands4b, 4, "iiIfF")),
BinaryenDrop(module, BinaryenLocalGet(module, 0, BinaryenTypeInt32())),
BinaryenLocalSet(module, 0, makeInt32(module, 101)),
BinaryenDrop(module, BinaryenLocalTee(module, 0, makeInt32(module, 102))),
BinaryenDrop(
module,
BinaryenLocalTee(module, 0, makeInt32(module, 102), BinaryenTypeInt32())),
BinaryenLoad(module, 4, 0, 0, 0, BinaryenTypeInt32(), makeInt32(module, 1)),
BinaryenLoad(module, 2, 1, 2, 1, BinaryenTypeInt64(), makeInt32(module, 8)),
BinaryenLoad(
Expand Down
2 changes: 1 addition & 1 deletion test/example/c-api-kitchen-sink.txt
Original file line number Diff line number Diff line change
Expand Up @@ -3835,7 +3835,7 @@ int main() {
expressions[747] = BinaryenConst(the_module, BinaryenLiteralInt32(101));
expressions[748] = BinaryenLocalSet(the_module, 0, expressions[747]);
expressions[749] = BinaryenConst(the_module, BinaryenLiteralInt32(102));
expressions[750] = BinaryenLocalTee(the_module, 0, expressions[749]);
expressions[750] = BinaryenLocalTee(the_module, 0, expressions[749], 2);
expressions[751] = BinaryenDrop(the_module, expressions[750]);
expressions[752] = BinaryenConst(the_module, BinaryenLiteralInt32(1));
expressions[753] = BinaryenLoad(the_module, 4, 0, 0, 0, 2, expressions[752]);
Expand Down
Loading