diff --git a/CHANGELOG.md b/CHANGELOG.md index b42efef386a..70b830357a5 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -21,6 +21,15 @@ Current Trunk - Add a new `BinaryenModuleReadWithFeatures` function to the C API that allows to configure which features to enable in the parser. + - Fix the result type of `br_if` to match the spec. This means that `br_if` now + returns a less precise type than before. In the IR it means that the type of + `br_if` with a value depends on the block it targets, which means it must be + provided when creating it, and updated if altered. + - As a result the C API `BinaryenBreak` now has a final parameter to allow + setting the type, which is a `Type*`. If NULL then the type will be + inferred in common cases (MVP types, or anything but a `br_if` with a + value). + v117 ---- diff --git a/src/binaryen-c.cpp b/src/binaryen-c.cpp index 29e0597d73b..7da3cc6ad5e 100644 --- a/src/binaryen-c.cpp +++ b/src/binaryen-c.cpp @@ -1120,10 +1120,21 @@ BinaryenExpressionRef BinaryenLoop(BinaryenModuleRef module, BinaryenExpressionRef BinaryenBreak(BinaryenModuleRef module, const char* name, BinaryenExpressionRef condition, - BinaryenExpressionRef value) { + BinaryenExpressionRef value, + BinaryenType* type) { + std::optional optType; + if (type) { + optType = Type(*type); + } else { + // As a convenience for users, if type is not provided but we can infer the + // type - which is the case for MVP types - then infer it. + if (condition && value && !value->type.containsRef()) { + optType = value->type; + } + } return static_cast( Builder(*(Module*)module) - .makeBreak(name, (Expression*)value, (Expression*)condition)); + .makeBreak(name, (Expression*)value, (Expression*)condition, optType)); } BinaryenExpressionRef BinaryenSwitch(BinaryenModuleRef module, const char** names, diff --git a/src/binaryen-c.h b/src/binaryen-c.h index 42bd4591573..0f120090584 100644 --- a/src/binaryen-c.h +++ b/src/binaryen-c.h @@ -740,12 +740,16 @@ BINARYEN_API BinaryenExpressionRef BinaryenIf(BinaryenModuleRef module, BINARYEN_API BinaryenExpressionRef BinaryenLoop(BinaryenModuleRef module, const char* in, BinaryenExpressionRef body); -// Break: value and condition can be NULL +// Break: value and condition can be NULL. type can be NULL, but must be +// provided for a `br_if` with a value (as the type depends on the block +// that is targeted). The type can also be NULL if the value type is an +// MVP type. BINARYEN_API BinaryenExpressionRef BinaryenBreak(BinaryenModuleRef module, const char* name, BinaryenExpressionRef condition, - BinaryenExpressionRef value); + BinaryenExpressionRef value, + BinaryenType* type); // Switch: value can be NULL BINARYEN_API BinaryenExpressionRef BinaryenSwitch(BinaryenModuleRef module, @@ -1265,6 +1269,9 @@ BINARYEN_API void BinaryenBreakSetCondition(BinaryenExpressionRef expr, BINARYEN_API BinaryenExpressionRef BinaryenBreakGetValue(BinaryenExpressionRef expr); // Sets the value expression, if any, of a `br` or `br_if` expression. +// Note that for a `br_if` with a value, if you change the type of the block +// that this targets then you also need to change the type of the `br_if`, which +// you can do using BinaryenExpressionSetType(). BINARYEN_API void BinaryenBreakSetValue(BinaryenExpressionRef expr, BinaryenExpressionRef valueExpr); diff --git a/src/ir/ReFinalize.cpp b/src/ir/ReFinalize.cpp index 6d872056adb..9c21f2d7e36 100644 --- a/src/ir/ReFinalize.cpp +++ b/src/ir/ReFinalize.cpp @@ -24,6 +24,51 @@ static Type getValueType(Expression* value) { return value ? value->type : Type::none; } +void ReFinalize::doWalkFunction(Function* func) { + using Super = + WalkerPass>>; + + while (1) { + // Walk normally. + Super::doWalkFunction(func); + + // After that walk we may have br_ifs in need of refinalization. Update them + // and refinalize again, as they may enable further improvements. This is in + // theory very slow, but in practice one or two cycles suffices and we can't + // try to be frugal here as must propagate all the possible improvements, or + // else we'd end up with a situation where ReFinalize^2 != ReFinalize. (If + // this ends up being slow in practice we'd want to build a custom graph IR + // for this operation here.) + // + // Note that this loop must surely terminate because the lattice of types is + // finite. + auto updatedBr = false; + + for (auto& [_, info] : blockBrInfoMap) { + auto* block = info.block; + assert(block->type.isConcrete()); + auto blockType = block->type; + for (auto* br : info.brs) { + // We would have ignored an unreachable or MVP br_if before; only + // references must be refined. + assert(br->type.containsRef()); + if (br->type != blockType) { + br->type = blockType; + updatedBr = true; + } + } + } + + if (!updatedBr) { + return; + } + + // Clear all state and loop/walk again. + breakTypes.clear(); + blockBrInfoMap.clear(); + } +} + void ReFinalize::visitBlock(Block* curr) { if (curr->list.size() == 0) { curr->type = Type::none; @@ -38,6 +83,12 @@ void ReFinalize::visitBlock(Block* curr) { auto& types = iter->second; types.insert(curr->list.back()->type); curr->type = Type::getLeastUpperBound(types); + + // If we have br_ifs then we must note ourselves there. + auto iter = blockBrInfoMap.find(curr->name); + if (iter != blockBrInfoMap.end()) { + iter->second.block = curr; + } return; } } @@ -65,6 +116,10 @@ void ReFinalize::visitBreak(Break* curr) { } else { updateBreakValueType(curr->name, valueType); } + // Note relevant br_ifs for type updating later. + if (curr->condition && curr->value && curr->type.containsRef()) { + blockBrInfoMap[curr->name].brs.push_back(curr); + } } void ReFinalize::visitSwitch(Switch* curr) { curr->finalize(); diff --git a/src/ir/utils.h b/src/ir/utils.h index 72aa701bb77..a0668d10287 100644 --- a/src/ir/utils.h +++ b/src/ir/utils.h @@ -129,10 +129,25 @@ struct ReFinalize ReFinalize() { name = "refinalize"; } - // block finalization is O(bad) if we do each block by itself, so do it in + // Block finalization is O(bad) if we do each block by itself, so do it in // bulk, tracking break value types so we just do a linear pass. std::unordered_map> breakTypes; + // Track br_ifs with values as we go, as those must be updated if their block + // type changes. We must also track block names to blocks for this, so we + // track all that using the block name as the key. + struct BlockBrInfo { + // The block and its relevant breaks. + Block* block = nullptr; + std::vector brs; + + BlockBrInfo() {} + BlockBrInfo(Block* block) : block(block) {} + }; + std::unordered_map blockBrInfoMap; + + void doWalkFunction(Function* func); + #define DELEGATE(CLASS_TO_VISIT) \ void visit##CLASS_TO_VISIT(CLASS_TO_VISIT* curr); diff --git a/src/passes/Heap2Local.cpp b/src/passes/Heap2Local.cpp index fcf0d88863b..04921f18236 100644 --- a/src/passes/Heap2Local.cpp +++ b/src/passes/Heap2Local.cpp @@ -343,8 +343,13 @@ struct Heap2LocalOptimizer { } // Breaks that our allocation flows through may change type, as we now - // have a nullable type there. - curr->finalize(); + // have a nullable type there. This is simple to fix as we know this is a + // break with a value, and the only possible change there is to become + // nullable. + assert(curr->value); + if (curr->type.isNonNullable()) { + curr->type = Type(curr->type.getHeapType(), Nullable); + } } void visitStructNew(StructNew* curr) { diff --git a/src/passes/RemoveUnusedBrs.cpp b/src/passes/RemoveUnusedBrs.cpp index 893cf9039bc..278d96590a1 100644 --- a/src/passes/RemoveUnusedBrs.cpp +++ b/src/passes/RemoveUnusedBrs.cpp @@ -140,6 +140,29 @@ static bool tooCostlyToRunUnconditionally(const PassOptions& passOptions, return tooCostlyToRunUnconditionally(passOptions, max); } +// Utility to update a br_if type after a change. This receives the br and a +// mapping of block names to their types, which it uses if it needs to (in +// particular, we do not need the mapping to figure out the type of a br_if +// when the value has MVP type, so the mapping can only contain things relevant +// to GC). This also calls finalize as needed, so that after calling it the +// br_if type is fully updated. +using BlockTypeMap = std::unordered_map; +static void updateBrIfType(Break* br, const BlockTypeMap& blockTypeMap) { + assert(br->condition); + assert(br->condition); + if (br->value) { + if (br->value->type.containsRef()) { + auto iter = blockTypeMap.find(br->name); + assert(iter != blockTypeMap.end()); + br->type = iter->second; + } else { + // A simple type we can infer from the value. + br->type = br->value->type; + } + } + br->finalize(); +} + struct RemoveUnusedBrs : public WalkerPass> { bool isFunctionParallel() override { return true; } @@ -251,11 +274,14 @@ struct RemoveUnusedBrs : public WalkerPass> { break; } } + // Otherwise it is ok for properly-typed values to flow out. + self->stopUnrefinedValueFlow(curr->type); } else if (curr->is()) { // ignore (could be result of a previous cycle) self->stopValueFlow(); } else if (curr->is()) { - // do nothing - it's ok for values to flow out + // As with a block, it is ok for properly-typed values to flow out. + self->stopUnrefinedValueFlow(curr->type); } else if (auto* sw = curr->dynCast()) { self->stopFlow(); self->optimizeSwitch(sw); @@ -282,6 +308,52 @@ struct RemoveUnusedBrs : public WalkerPass> { void stopValueFlow() { removeValueFlow(flows); } + // Stop a value from flowing that is not as refined as a given type. For + // example: + // + // (block (result $sub) + // ..maybe other branches to the block.. + // (return (super)) ;; the function returns super; the block is sub + // ) + // + // If we let the value flow through, we'd be forced to unrefine the block: + // + // (block (result $super) + // ..maybe other branches to the block.. + // (super) + // ) + // + // It is best to avoid unrefining like that, so block the flow of such values + // here. + void stopUnrefinedValueFlow(Type type) { + if (type == Type::unreachable) { + // We are flowing through something currently unreachable. Us flowing + // through it may turn it reachable, which is fine, and there are no + // subtyping risks here, so nothing to check. + return; + } + if (!type.containsRef()) { + // Subtyping cannot be an issue here. + return; + } + flows.erase(std::remove_if(flows.begin(), + flows.end(), + [&](Expression** currp) { + auto* curr = *currp; + Expression* value; + if (auto* ret = curr->dynCast()) { + value = ret->value; + } else { + value = curr->cast()->value; + } + // Remove if there is a value, and it is + // unrefined. + return value && + !Type::isSubType(value->type, type); + }), + flows.end()); + } + static void clear(RemoveUnusedBrs* self, Expression** currp) { self->flows.clear(); } @@ -417,7 +489,7 @@ struct RemoveUnusedBrs : public WalkerPass> { br->condition = builder.makeSelect(br->condition, curr->condition, zero); } - br->finalize(); + updateBrIfType(br, blockTypes); replaceCurrent(Builder(*getModule()).dropIfConcretelyTyped(br)); anotherCycle = true; } @@ -460,9 +532,9 @@ struct RemoveUnusedBrs : public WalkerPass> { static void scan(RemoveUnusedBrs* self, Expression** currp) { self->pushTask(visitAny, currp); - auto* iff = (*currp)->dynCast(); + auto* curr = *currp; - if (iff) { + if (auto* iff = curr->dynCast()) { if (iff->condition->type == Type::unreachable) { // avoid trying to optimize this, we never reach it anyhow return; @@ -478,11 +550,23 @@ struct RemoveUnusedBrs : public WalkerPass> { self->pushTask(scan, &iff->ifTrue); self->pushTask(clear, currp); // clear all flow after the condition self->pushTask(scan, &iff->condition); - } else { - super::scan(self, currp); + return; + } + + if (auto* block = curr->dynCast()) { + if (block->name.is() && block->type.containsRef()) { + // This block has a type that may be needed for updating br_ifs, so + // note it. + self->blockTypes[block->name] = block->type; + } } + + super::scan(self, currp); } + // We track the types of blocks that have relevant types for updateBrIfType. + std::unordered_map blockTypes; + // optimizes a loop. returns true if we made changes bool optimizeLoop(Loop* loop) { // if a loop ends in @@ -1066,7 +1150,7 @@ struct RemoveUnusedBrs : public WalkerPass> { // we are an if-else where the ifTrue is a break without a // condition, so we can do this ifTrueBreak->condition = iff->condition; - ifTrueBreak->finalize(); + updateBrIfType(ifTrueBreak, blockTypes); list[i] = Builder(*getModule()).dropIfConcretelyTyped(ifTrueBreak); ExpressionManipulator::spliceIntoBlock(curr, i + 1, iff->ifFalse); continue; @@ -1080,7 +1164,7 @@ struct RemoveUnusedBrs : public WalkerPass> { *getModule())) { ifFalseBreak->condition = Builder(*getModule()).makeUnary(EqZInt32, iff->condition); - ifFalseBreak->finalize(); + updateBrIfType(ifFalseBreak, blockTypes); list[i] = Builder(*getModule()).dropIfConcretelyTyped(ifFalseBreak); ExpressionManipulator::spliceIntoBlock(curr, i + 1, iff->ifTrue); continue; @@ -1676,6 +1760,21 @@ struct RemoveUnusedBrs : public WalkerPass> { start = end; } } + + // Note types of blocks. This parallels the logic in the parent class, + // but we do not want to just reuse the data structure there: things may + // have changed since then. + static void scan(FinalOptimizer* self, Expression** currp) { + if (auto* block = (*currp)->dynCast()) { + if (block->name.is() && block->type.containsRef()) { + self->blockTypes[block->name] = block->type; + } + } + + PostWalker::scan(self, currp); + } + + std::unordered_map blockTypes; }; FinalOptimizer finalOptimizer(getPassOptions()); finalOptimizer.setModule(getModule()); diff --git a/src/passes/SimplifyLocals.cpp b/src/passes/SimplifyLocals.cpp index 28b13980b2a..db852f01f88 100644 --- a/src/passes/SimplifyLocals.cpp +++ b/src/passes/SimplifyLocals.cpp @@ -608,11 +608,14 @@ struct SimplifyLocals auto* set = (*breakLocalSetPointer)->template cast(); if (br->condition) { br->value = set; - set->makeTee(this->getFunction()->getLocalType(set->index)); + auto localType = this->getFunction()->getLocalType(set->index); + set->makeTee(localType); *breakLocalSetPointer = this->getModule()->allocator.template alloc(); - // in addition, as this is a conditional br that now has a value, it now - // returns a value, so it must be dropped + // In addition, as this is a conditional br that now has a value, it now + // returns a value, so we must update the type for that and also drop + // it. + br->type = localType; br->finalize(); *brp = Builder(*this->getModule()).makeDrop(br); } else { diff --git a/src/tools/fuzzing/fuzzing.cpp b/src/tools/fuzzing/fuzzing.cpp index 9a44119f56b..dce970becf7 100644 --- a/src/tools/fuzzing/fuzzing.cpp +++ b/src/tools/fuzzing/fuzzing.cpp @@ -1536,7 +1536,7 @@ Expression* TranslateToFuzzReader::makeBreak(Type type) { // we need to break to a proper place continue; } - auto* ret = builder.makeBreak(name, make(type), condition); + auto* ret = builder.makeBreak(name, make(type), condition, valueType); funcContext->hangStack.pop_back(); return ret; } else if (type == Type::none) { diff --git a/src/wasm-builder.h b/src/wasm-builder.h index eb1874c3b81..ffcba650a4d 100644 --- a/src/wasm-builder.h +++ b/src/wasm-builder.h @@ -239,11 +239,21 @@ class Builder { } Break* makeBreak(Name name, Expression* value = nullptr, - Expression* condition = nullptr) { + Expression* condition = nullptr, + std::optional type = std::nullopt) { auto* ret = wasm.allocator.alloc(); ret->name = name; ret->value = value; ret->condition = condition; + if (value && condition) { + // This is a br_if, for whom we must receive the type, as the type must + // match the block it targets (which we do not know here). + // + // Note that we could infer the type in some cases, like for MVP types, + // but we intentionally do not so as to catch bugs earlier. + assert(type); + ret->type = *type; + } ret->finalize(); return ret; } diff --git a/src/wasm-s-parser.h b/src/wasm-s-parser.h index e1d9dab47ee..5853a9e65c7 100644 --- a/src/wasm-s-parser.h +++ b/src/wasm-s-parser.h @@ -206,6 +206,12 @@ class SExpressionWasmBuilder { bool isType(IString str) { return stringToType(str, true) != Type::none; } HeapType getFunctionType(Name name, Element& s); + // Maps block names to their types. This is built up as we go so that things + // like br_if can query the type of the thing they target. Note that we don't + // note the types of loops because breaks to them must not send a value + // anyhow. + std::unordered_map blockTypes; + public: Expression* parseExpression(Element* s) { return parseExpression(*s); } Expression* parseExpression(Element& s); diff --git a/src/wasm-type.h b/src/wasm-type.h index 5c47f405191..176a48da7aa 100644 --- a/src/wasm-type.h +++ b/src/wasm-type.h @@ -173,6 +173,8 @@ class Type { bool isException() const; bool isString() const; bool isDefaultable() const; + // The same as isRef() but also returns true for a tuple containing a ref. + bool containsRef() const; Nullability getNullability() const; diff --git a/src/wasm/wasm-binary.cpp b/src/wasm/wasm-binary.cpp index efb65bb5095..b14148a9658 100644 --- a/src/wasm/wasm-binary.cpp +++ b/src/wasm/wasm-binary.cpp @@ -4513,6 +4513,10 @@ void WasmBinaryReader::visitBreak(Break* curr, uint8_t code) { } if (target.type.isConcrete()) { curr->value = popTypedExpression(target.type); + if (curr->condition) { + // This is a br_if; set the type based on the target. + curr->type = target.type; + } } curr->finalize(); } diff --git a/src/wasm/wasm-ir-builder.cpp b/src/wasm/wasm-ir-builder.cpp index 4bc80d25c23..fcac7702722 100644 --- a/src/wasm/wasm-ir-builder.cpp +++ b/src/wasm/wasm-ir-builder.cpp @@ -1114,7 +1114,14 @@ Result<> IRBuilder::makeBreak(Index label, bool isConditional) { // Use a dummy condition value if we need to pop a condition. curr.condition = isConditional ? &curr : nullptr; CHECK_ERR(visitBreak(&curr, label)); - push(builder.makeBreak(curr.name, curr.value, curr.condition)); + // If this is a br_if then we must provide the type (which is the type of the + // block we target). + std::optional type; + if (curr.value && curr.condition) { + // N.B. the label was already validated above when we did getLabelName. + type = scopeStack[scopeStack.size() - label - 1].getResultType(); + } + push(builder.makeBreak(curr.name, curr.value, curr.condition, type)); return Ok{}; } diff --git a/src/wasm/wasm-s-parser.cpp b/src/wasm/wasm-s-parser.cpp index 44ba8435113..5ebbd8da374 100644 --- a/src/wasm/wasm-s-parser.cpp +++ b/src/wasm/wasm-s-parser.cpp @@ -1649,6 +1649,7 @@ Expression* SExpressionWasmBuilder::makeBlock(Element& s) { curr->name = nameMapper.pushLabelName(sName); // block signature curr->type = parseBlockType(s, i); + blockTypes[curr->name] = curr->type; if (i >= s.size()) { break; // empty block } @@ -2599,6 +2600,19 @@ Expression* SExpressionWasmBuilder::makeBreak(Element& s, bool isConditional) { } else { ret->value = parseExpression(s[i]); } + if (isConditional && ret->value) { + auto iter = blockTypes.find(ret->name); + if (iter == blockTypes.end()) { + // There is no block by that name, so this must target the function scope. + // We could also validate that it is in fact the function scope, but the + // main validator does that anyhow; all we need here is to generate valid + // IR if it is valid. + ret->type = currFunction->getResults(); + } else { + // Get the type from the block. + ret->type = iter->second; + } + } ret->finalize(); return ret; } diff --git a/src/wasm/wasm-type.cpp b/src/wasm/wasm-type.cpp index b14e5aa39d2..69693672cdc 100644 --- a/src/wasm/wasm-type.cpp +++ b/src/wasm/wasm-type.cpp @@ -824,6 +824,18 @@ bool Type::isDefaultable() const { return isConcrete() && !isNonNullable(); } +bool Type::containsRef() const { + if (isTuple()) { + for (auto t : *this) { + if (t.isRef()) { + return true; + } + } + return false; + } + return isRef(); +} + Nullability Type::getNullability() const { return isNullable() ? Nullable : NonNullable; } diff --git a/src/wasm/wasm-validator.cpp b/src/wasm/wasm-validator.cpp index 4bb5561604a..cb32ad225d3 100644 --- a/src/wasm/wasm-validator.cpp +++ b/src/wasm/wasm-validator.cpp @@ -247,6 +247,9 @@ struct FunctionValidator : public WalkerPass> { void noteLabelName(Name name); + // Maps a label name (block etc.) to the br_ifs targetting it. + std::unordered_map> labelBrIfs; + public: // visitors @@ -652,8 +655,8 @@ void FunctionValidator::visitBlock(Block* curr) { curr, "Multivalue block type require multivalue [--enable-multivalue]"); } - // if we are break'ed to, then the value must be right for us if (curr->name.is()) { + // If we are break'ed to, then the value must be right for us noteLabelName(curr->name); auto iter = breakTypes.find(curr->name); assert(iter != breakTypes.end()); // we set it ourselves @@ -669,6 +672,19 @@ void FunctionValidator::visitBlock(Block* curr) { "break type must be a subtype of the target block type"); } breakTypes.erase(iter); + + // Validate br_if types as matching this block's type. + if (auto iter = labelBrIfs.find(curr->name); iter != labelBrIfs.end()) { + for (auto* brIf : iter->second) { + // This must be an actual br_if with a value. + assert(brIf->value && brIf->condition); + shouldBeEqualOrFirstIsUnreachable( + brIf->type, + curr->type, + brIf, + "br_ifs to a block must have the block's type"); + } + } } switch (getFunction()->profile) { case IRProfile::Normal: @@ -781,6 +797,11 @@ void FunctionValidator::visitLoop(Loop* curr) { "breaks to a loop cannot pass a value"); } breakTypes.erase(iter); + + // Validate there are no br_ifs with values targeting us. + shouldBeTrue(!labelBrIfs.count(curr->name), + curr, + "loops cannot have br_ifs with values targeting them"); } if (curr->type == Type::none) { shouldBeFalse(curr->body->type.isConcrete(), @@ -890,6 +911,17 @@ void FunctionValidator::visitBreak(Break* curr) { curr, "break condition must be i32"); } + if (curr->condition && curr->value) { + labelBrIfs[curr->name].push_back(curr); + } + // An unreachable br must have no condition, or it must have an unreachable + // child. + if (curr->type == Type::unreachable && curr->condition) { + shouldBeTrue(curr->condition->type == Type::unreachable || + (curr->value && curr->value->type == Type::unreachable), + curr, + "unreachable break with condition or no unreachable child"); + } } void FunctionValidator::visitSwitch(Switch* curr) { diff --git a/src/wasm/wasm.cpp b/src/wasm/wasm.cpp index 374a97e1902..9c0002b8c1c 100644 --- a/src/wasm/wasm.cpp +++ b/src/wasm/wasm.cpp @@ -252,14 +252,13 @@ void Break::finalize() { if (condition->type == Type::unreachable) { type = Type::unreachable; } else if (value) { - // N.B. This is not correct wrt the spec, which mandates that it be the - // type of the block we target. In practice this does not matter because - // the br_if return value is not really used in the wild. To fix this, - // we'd need to do something like what we do for local.tee's type, which - // is to fix it up in a way that is aware of function-level context and - // not just the instruction itself (which would be a pain). - type = value->type; - } else { + if (value->type == Type::unreachable) { + type = Type::unreachable; + } + // Note that we do nothing for a reachable br_if. Its type must match the + // type of the block it targets, which we do not see here, so it is up to + // optimization passes to manually update it as (rarely) needed. + } else if (!value) { type = Type::none; } } else { diff --git a/test/example/c-api-kitchen-sink.c b/test/example/c-api-kitchen-sink.c index e5b614becbe..b1bb30bb35d 100644 --- a/test/example/c-api-kitchen-sink.c +++ b/test/example/c-api-kitchen-sink.c @@ -1000,10 +1000,13 @@ void test_core() { BinaryenIf(module, temp4, temp5, NULL), BinaryenLoop(module, "in", makeInt32(module, 0)), BinaryenLoop(module, NULL, makeInt32(module, 0)), - BinaryenBreak(module, "the-value", temp6, temp7), - BinaryenBreak(module, "the-nothing", makeInt32(module, 2), NULL), - BinaryenBreak(module, "the-value", NULL, makeInt32(module, 3)), - BinaryenBreak(module, "the-nothing", NULL, NULL), + BinaryenBreak(module, "the-value", temp6, temp7, &i32), + // We can also provide NULL as the type of the br_if, as it is an MVP type + // that is inferred. + BinaryenBreak(module, "the-value", temp6, temp7, NULL), + BinaryenBreak(module, "the-nothing", makeInt32(module, 2), NULL, NULL), + BinaryenBreak(module, "the-value", NULL, makeInt32(module, 3), NULL), + BinaryenBreak(module, "the-nothing", NULL, NULL, NULL), BinaryenSwitch(module, switchValueNames, 1, "the-value", temp8, temp9), BinaryenSwitch( module, switchBodyNames, 1, "the-nothing", makeInt32(module, 2), NULL), diff --git a/test/example/c-api-kitchen-sink.txt b/test/example/c-api-kitchen-sink.txt index 469267a4dd1..2c1e802c0bb 100644 --- a/test/example/c-api-kitchen-sink.txt +++ b/test/example/c-api-kitchen-sink.txt @@ -2007,6 +2007,12 @@ BinaryenFeatureAll: 131071 (i32.const 0) ) ) + (drop + (br_if $the-value + (i32.const 1) + (i32.const 0) + ) + ) (br_if $the-nothing (i32.const 2) ) diff --git a/test/lit/passes/flatten_all-features.wast b/test/lit/passes/flatten_all-features.wast index aa3dc7578d8..e65e5b79f76 100644 --- a/test/lit/passes/flatten_all-features.wast +++ b/test/lit/passes/flatten_all-features.wast @@ -3586,13 +3586,13 @@ ;; value type, we need the value to be set into two locals: one with the outer ;; block's type, and one with its value type. ;; CHECK: (func $subtype (type $6) (result anyref) - ;; CHECK-NEXT: (local $0 eqref) + ;; CHECK-NEXT: (local $0 anyref) ;; CHECK-NEXT: (local $1 anyref) ;; CHECK-NEXT: (local $2 nullref) ;; CHECK-NEXT: (local $3 nullref) - ;; CHECK-NEXT: (local $4 eqref) - ;; CHECK-NEXT: (local $5 eqref) - ;; CHECK-NEXT: (local $6 eqref) + ;; CHECK-NEXT: (local $4 anyref) + ;; CHECK-NEXT: (local $5 anyref) + ;; CHECK-NEXT: (local $6 anyref) ;; CHECK-NEXT: (local $7 anyref) ;; CHECK-NEXT: (block $label0 ;; CHECK-NEXT: (block @@ -3633,9 +3633,9 @@ ;; CHECK-NEXT: ) ;; CHECK-NEXT: ) (func $subtype (result anyref) - (local $0 eqref) + (local $0 anyref) (block $label0 (result anyref) - (block (result eqref) + (block (result anyref) (local.tee $0 (br_if $label0 (ref.null eq) diff --git a/test/lit/passes/heap2local.wast b/test/lit/passes/heap2local.wast index b693c49ee95..acf624bfca1 100644 --- a/test/lit/passes/heap2local.wast +++ b/test/lit/passes/heap2local.wast @@ -1952,6 +1952,45 @@ ) ) ) + + ;; CHECK: (func $br_if (type $1) + ;; CHECK-NEXT: (local $0 i32) + ;; CHECK-NEXT: (local $1 f64) + ;; CHECK-NEXT: (drop + ;; CHECK-NEXT: (block $label$1 (result (ref null $struct.A)) + ;; CHECK-NEXT: (drop + ;; CHECK-NEXT: (br_if $label$1 + ;; CHECK-NEXT: (block (result nullref) + ;; CHECK-NEXT: (local.set $0 + ;; CHECK-NEXT: (i32.const 0) + ;; CHECK-NEXT: ) + ;; CHECK-NEXT: (local.set $1 + ;; CHECK-NEXT: (f64.const 0) + ;; CHECK-NEXT: ) + ;; CHECK-NEXT: (ref.null none) + ;; CHECK-NEXT: ) + ;; CHECK-NEXT: (i32.const 0) + ;; CHECK-NEXT: ) + ;; CHECK-NEXT: ) + ;; CHECK-NEXT: (unreachable) + ;; CHECK-NEXT: ) + ;; CHECK-NEXT: ) + ;; CHECK-NEXT: ) + (func $br_if + ;; The br_if here must change type after we optimize, as then a nullable + ;; type will be flowing through it. + (drop + (block $label$1 (result (ref $struct.A)) + (drop + (br_if $label$1 + (struct.new_default $struct.A) + (i32.const 0) + ) + ) + (unreachable) + ) + ) + ) ) (module diff --git a/test/lit/passes/precompute-gc.wast b/test/lit/passes/precompute-gc.wast index 2b16cb43f95..57e24c3f7bb 100644 --- a/test/lit/passes/precompute-gc.wast +++ b/test/lit/passes/precompute-gc.wast @@ -22,7 +22,7 @@ (type $func-return-i32 (func (result i32))) - ;; CHECK: (import "fuzzing-support" "log-i32" (func $log (type $4) (param i32))) + ;; CHECK: (import "fuzzing-support" "log-i32" (func $log (type $5) (param i32))) (import "fuzzing-support" "log-i32" (func $log (param i32))) ;; CHECK: (func $test-fallthrough (type $func-return-i32) (result i32) @@ -119,7 +119,7 @@ (struct.get $struct 0 (local.get $x)) ) ) - ;; CHECK: (func $load-from-struct-bad-merge (type $4) (param $i i32) + ;; CHECK: (func $load-from-struct-bad-merge (type $5) (param $i i32) ;; CHECK-NEXT: (local $x (ref null $struct)) ;; CHECK-NEXT: (if ;; CHECK-NEXT: (local.get $i) @@ -168,7 +168,7 @@ (struct.get $struct 0 (local.get $x)) ) ) - ;; CHECK: (func $modify-gc-heap (type $5) (param $x (ref null $struct)) + ;; CHECK: (func $modify-gc-heap (type $6) (param $x (ref null $struct)) ;; CHECK-NEXT: (struct.set $struct 0 ;; CHECK-NEXT: (local.get $x) ;; CHECK-NEXT: (i32.add @@ -222,7 +222,7 @@ (struct.get $struct 0 (local.get $x)) ) ) - ;; CHECK: (func $load-from-struct-bad-arrive (type $5) (param $x (ref null $struct)) + ;; CHECK: (func $load-from-struct-bad-arrive (type $6) (param $x (ref null $struct)) ;; CHECK-NEXT: (call $log ;; CHECK-NEXT: (struct.get $struct 0 ;; CHECK-NEXT: (local.get $x) @@ -388,7 +388,7 @@ (local.get $tempresult) ) - ;; CHECK: (func $propagate-uncertain-param (type $6) (param $input (ref $empty)) (result i32) + ;; CHECK: (func $propagate-uncertain-param (type $7) (param $input (ref $empty)) (result i32) ;; CHECK-NEXT: (local $tempresult i32) ;; CHECK-NEXT: (local $tempref (ref null $empty)) ;; CHECK-NEXT: (local.set $tempresult @@ -435,7 +435,7 @@ (local.get $tempresult) ) - ;; CHECK: (func $propagate-same-param (type $6) (param $input (ref $empty)) (result i32) + ;; CHECK: (func $propagate-same-param (type $7) (param $input (ref $empty)) (result i32) ;; CHECK-NEXT: (local $tempresult i32) ;; CHECK-NEXT: (local.set $tempresult ;; CHECK-NEXT: (ref.eq @@ -850,7 +850,7 @@ ) ) - ;; CHECK: (func $new_block_unreachable (type $8) (result anyref) + ;; CHECK: (func $new_block_unreachable (type $9) (result anyref) ;; CHECK-NEXT: (block ;; (replaces unreachable StructNew we can't emit) ;; CHECK-NEXT: (drop ;; CHECK-NEXT: (block @@ -892,7 +892,7 @@ ) ) - ;; CHECK: (func $ref.is_null (type $4) (param $param i32) + ;; CHECK: (func $ref.is_null (type $5) (param $param i32) ;; CHECK-NEXT: (local $ref (ref null $empty)) ;; CHECK-NEXT: (local.set $ref ;; CHECK-NEXT: (struct.new_default $empty) @@ -1052,7 +1052,7 @@ ) ) - ;; CHECK: (func $get-nonnullable-in-unreachable (type $8) (result anyref) + ;; CHECK: (func $get-nonnullable-in-unreachable (type $9) (result anyref) ;; CHECK-NEXT: (local $x (ref any)) ;; CHECK-NEXT: (local.tee $x ;; CHECK-NEXT: (unreachable) @@ -1089,7 +1089,7 @@ (local.get $x) ) - ;; CHECK: (func $get-nonnullable-in-unreachable-entry (type $9) (param $x i32) (param $y (ref any)) + ;; CHECK: (func $get-nonnullable-in-unreachable-entry (type $10) (param $x i32) (param $y (ref any)) ;; CHECK-NEXT: (local $0 (ref any)) ;; CHECK-NEXT: (unreachable) ;; CHECK-NEXT: (local.set $0 @@ -1123,7 +1123,7 @@ ) ) - ;; CHECK: (func $get-nonnullable-in-unreachable-later-loop (type $9) (param $x i32) (param $y (ref any)) + ;; CHECK: (func $get-nonnullable-in-unreachable-later-loop (type $10) (param $x i32) (param $y (ref any)) ;; CHECK-NEXT: (local $0 (ref any)) ;; CHECK-NEXT: (if ;; CHECK-NEXT: (local.get $x) @@ -1196,4 +1196,79 @@ ) (local.get $x) ) + + ;; CHECK: (func $refinalize-refine-br_if (type $20) (param $nn-any (ref any)) (result anyref) + ;; CHECK-NEXT: (nop) + ;; CHECK-NEXT: (block $block (result (ref any)) + ;; CHECK-NEXT: (drop + ;; CHECK-NEXT: (block (result (ref any)) + ;; CHECK-NEXT: (br_if $block + ;; CHECK-NEXT: (local.get $nn-any) + ;; CHECK-NEXT: (i32.const 1) + ;; CHECK-NEXT: ) + ;; CHECK-NEXT: ) + ;; CHECK-NEXT: ) + ;; CHECK-NEXT: (local.get $nn-any) + ;; CHECK-NEXT: ) + ;; CHECK-NEXT: ) + (func $refinalize-refine-br_if (param $nn-any (ref any)) (result anyref) + ;; A nop prevents the next block from being elided in printing. + (nop) + ;; This tests refinalize (using precompute, because it always refinalizes). + ;; When we refine the block to non-nullable we must also update the br_if to + ;; the same type, as they must match. + (block $block (result anyref) + (drop + (block $ignore (result anyref) ;; This block will also get refined, after the br_if. + (br_if $block + (local.get $nn-any) + (i32.const 1) + ) + ) + ) + (local.get $nn-any) + ) + ) + + ;; CHECK: (func $refinalize-refine-br_if-tuple (type $21) (param $nn-any (ref any)) (result anyref i32) + ;; CHECK-NEXT: (nop) + ;; CHECK-NEXT: (block $block (type $4) (result (ref any) i32) + ;; CHECK-NEXT: (tuple.drop 2 + ;; CHECK-NEXT: (block (type $4) (result (ref any) i32) + ;; CHECK-NEXT: (br_if $block + ;; CHECK-NEXT: (tuple.make 2 + ;; CHECK-NEXT: (local.get $nn-any) + ;; CHECK-NEXT: (i32.const 0) + ;; CHECK-NEXT: ) + ;; CHECK-NEXT: (i32.const 1) + ;; CHECK-NEXT: ) + ;; CHECK-NEXT: ) + ;; CHECK-NEXT: ) + ;; CHECK-NEXT: (tuple.make 2 + ;; CHECK-NEXT: (local.get $nn-any) + ;; CHECK-NEXT: (i32.const 2) + ;; CHECK-NEXT: ) + ;; CHECK-NEXT: ) + ;; CHECK-NEXT: ) + (func $refinalize-refine-br_if-tuple (param $nn-any (ref any)) (result anyref i32) + ;; As above, but with a tuple containing a reference. + (nop) + (block $block (result anyref i32) + (tuple.drop 2 + (block $ignore (result anyref i32) + (br_if $block + (tuple.make 2 + (local.get $nn-any) + (i32.const 0) + ) + (i32.const 1) + ) + ) + ) + (tuple.make 2 + (local.get $nn-any) + (i32.const 2) + ) + ) + ) ) diff --git a/test/lit/passes/remove-unused-brs-gc.wast b/test/lit/passes/remove-unused-brs-gc.wast index 9dc871e326b..2588973a91d 100644 --- a/test/lit/passes/remove-unused-brs-gc.wast +++ b/test/lit/passes/remove-unused-brs-gc.wast @@ -7,13 +7,18 @@ ;; CHECK: (rec ;; CHECK-NEXT: (type $struct (sub (struct ))) (type $struct (sub (struct))) + ;; CHECK: (type $struct2 (struct )) (type $struct2 (struct)) + ;; CHECK: (type $substruct (sub $struct (struct ))) (type $substruct (sub $struct (struct))) + + ;; CHECK: (type $subsubstruct (sub $substruct (struct ))) + (type $subsubstruct (sub $substruct (struct))) ) - ;; CHECK: (func $br_on-if (type $7) (param $0 (ref struct)) + ;; CHECK: (func $br_on-if (type $8) (param $0 (ref struct)) ;; CHECK-NEXT: (block $label ;; CHECK-NEXT: (drop ;; CHECK-NEXT: (select (result (ref struct)) @@ -48,7 +53,7 @@ ) ) - ;; CHECK: (func $br_on_cast (type $4) (result (ref $struct)) + ;; CHECK: (func $br_on_cast (type $6) (result (ref $struct)) ;; CHECK-NEXT: (local $struct (ref null $struct)) ;; CHECK-NEXT: (block $block (result (ref $struct)) ;; CHECK-NEXT: (drop @@ -99,7 +104,7 @@ ) ) - ;; CHECK: (func $br_on_cast-fallthrough (type $4) (result (ref $struct)) + ;; CHECK: (func $br_on_cast-fallthrough (type $6) (result (ref $struct)) ;; CHECK-NEXT: (local $struct (ref null $struct)) ;; CHECK-NEXT: (local $any anyref) ;; CHECK-NEXT: (block $block (result (ref $struct)) @@ -162,7 +167,7 @@ ) ) - ;; CHECK: (func $nested_br_on_cast (type $8) (result i31ref) + ;; CHECK: (func $nested_br_on_cast (type $9) (result i31ref) ;; CHECK-NEXT: (block $label$1 (result (ref i31)) ;; CHECK-NEXT: (drop ;; CHECK-NEXT: (br $label$1 @@ -190,7 +195,7 @@ ) ) - ;; CHECK: (func $br_on_cast_unrelated (type $5) (result (ref null $struct)) + ;; CHECK: (func $br_on_cast_unrelated (type $4) (result (ref null $struct)) ;; CHECK-NEXT: (local $nullable-struct2 (ref null $struct2)) ;; CHECK-NEXT: (block $block (result nullref) ;; CHECK-NEXT: (drop @@ -244,7 +249,7 @@ ) ) - ;; CHECK: (func $br_on_cast_unrelated-fallthrough (type $5) (result (ref null $struct)) + ;; CHECK: (func $br_on_cast_unrelated-fallthrough (type $4) (result (ref null $struct)) ;; CHECK-NEXT: (local $any anyref) ;; CHECK-NEXT: (local $nullable-struct2 (ref null $struct2)) ;; CHECK-NEXT: (block $block (result nullref) @@ -307,7 +312,7 @@ ) ) - ;; CHECK: (func $br_on_cast_fail (type $3) (result anyref) + ;; CHECK: (func $br_on_cast_fail (type $5) (result anyref) ;; CHECK-NEXT: (local $struct (ref null $struct)) ;; CHECK-NEXT: (block $block (result (ref null $struct)) ;; CHECK-NEXT: (drop @@ -354,7 +359,7 @@ ) ) - ;; CHECK: (func $br_on_cast_fail-fallthrough (type $3) (result anyref) + ;; CHECK: (func $br_on_cast_fail-fallthrough (type $5) (result anyref) ;; CHECK-NEXT: (local $any anyref) ;; CHECK-NEXT: (local $struct (ref null $struct)) ;; CHECK-NEXT: (block $block (result anyref) @@ -410,7 +415,7 @@ ) ) - ;; CHECK: (func $br_on_cast_fail_unrelated (type $3) (result anyref) + ;; CHECK: (func $br_on_cast_fail_unrelated (type $5) (result anyref) ;; CHECK-NEXT: (local $nullable-struct2 (ref null $struct2)) ;; CHECK-NEXT: (block $block (result (ref null $struct2)) ;; CHECK-NEXT: (drop @@ -472,7 +477,7 @@ ) ) - ;; CHECK: (func $br_on_cast_fail_unrelated-fallthrough (type $3) (result anyref) + ;; CHECK: (func $br_on_cast_fail_unrelated-fallthrough (type $5) (result anyref) ;; CHECK-NEXT: (local $any anyref) ;; CHECK-NEXT: (local $nullable-struct2 (ref null $struct2)) ;; CHECK-NEXT: (block $block (result anyref) @@ -543,7 +548,7 @@ ) ) - ;; CHECK: (func $br_on_cast-unreachable (type $6) (param $i31ref i31ref) (result anyref) + ;; CHECK: (func $br_on_cast-unreachable (type $7) (param $i31ref i31ref) (result anyref) ;; CHECK-NEXT: (block $block ;; CHECK-NEXT: (drop ;; CHECK-NEXT: (block @@ -599,7 +604,7 @@ ) ) - ;; CHECK: (func $fallthrough-unreachable (type $6) (param $0 i31ref) (result anyref) + ;; CHECK: (func $fallthrough-unreachable (type $7) (param $0 i31ref) (result anyref) ;; CHECK-NEXT: (block $outer ;; CHECK-NEXT: (drop ;; CHECK-NEXT: (block ;; (replaces unreachable RefCast we can't emit) @@ -638,7 +643,7 @@ ) ) - ;; CHECK: (func $casts-are-costly (type $9) (param $x i32) + ;; CHECK: (func $casts-are-costly (type $10) (param $x i32) ;; CHECK-NEXT: (local $struct (ref null $struct)) ;; CHECK-NEXT: (drop ;; CHECK-NEXT: (if (result i32) @@ -782,4 +787,61 @@ ) ) ) -) + + ;; CHECK: (func $never-unrefine (type $4) (result (ref null $struct)) + ;; CHECK-NEXT: (block $block (result (ref $substruct)) + ;; CHECK-NEXT: (nop) + ;; CHECK-NEXT: (return + ;; CHECK-NEXT: (struct.new_default $struct) + ;; CHECK-NEXT: ) + ;; CHECK-NEXT: ) + ;; CHECK-NEXT: ) + (func $never-unrefine (result (ref null $struct)) + ;; This block is more refined than the function's result, and we should not + ;; unrefine it. Instead we should keep the |return| in place (other opts might + ;; end up optimizing such code, e.g. vacuum). + ;; + ;; In this trivial case the block would be removed by vacuum anyhow, but in + ;; other situations it can be harmful to unrefine; for example, if there are + ;; br_ifs targeting the block as well, then they would end up unrefined too. + (block $block (result (ref $substruct)) + (nop) ;; avoid this being a trivial block + (return + (struct.new_default $struct) + ) + ) + ) + + ;; CHECK: (func $never-unrefine-ok (type $4) (result (ref null $struct)) + ;; CHECK-NEXT: (block $block (result (ref $substruct)) + ;; CHECK-NEXT: (nop) + ;; CHECK-NEXT: (struct.new_default $substruct) + ;; CHECK-NEXT: ) + ;; CHECK-NEXT: ) + (func $never-unrefine-ok (result (ref null $struct)) + ;; As above, but now we send in an equal type. We can remove the |return| + ;; here. + (block $block (result (ref $substruct)) + (nop) + (return + (struct.new_default $substruct) ;; this changed + ) + ) + ) + + ;; CHECK: (func $never-unrefine-ok-2 (type $4) (result (ref null $struct)) + ;; CHECK-NEXT: (block $block (result (ref $subsubstruct)) + ;; CHECK-NEXT: (nop) + ;; CHECK-NEXT: (struct.new_default $subsubstruct) + ;; CHECK-NEXT: ) + ;; CHECK-NEXT: ) + (func $never-unrefine-ok-2 (result (ref null $struct)) + ;; As above, but now we send in a strict subtype. In this case we end up + ;; refining the block (and also removing the |return|). + (block $block (result (ref $substruct)) + (nop) + (return + (struct.new_default $subsubstruct) ;; this changed + ) + ) + ))