diff --git a/scripts/fuzz_opt.py b/scripts/fuzz_opt.py index 5c06ef33995..84359998182 100755 --- a/scripts/fuzz_opt.py +++ b/scripts/fuzz_opt.py @@ -515,10 +515,10 @@ def compare_between_vms(x, y, context): y_line = y_lines[i] if x_line != y_line: # this is different, but maybe it's a vm difference we can ignore - LEI_LOGGING = '[LoggingExternalInterface logging' - if x_line.startswith(LEI_LOGGING) and y_line.startswith(LEI_LOGGING): - x_val = x_line[len(LEI_LOGGING) + 1:-1] - y_val = y_line[len(LEI_LOGGING) + 1:-1] + LOGGING_PREFIX = '[LoggingExternalInterface logging' + if x_line.startswith(LOGGING_PREFIX) and y_line.startswith(LOGGING_PREFIX): + x_val = x_line[len(LOGGING_PREFIX) + 1:-1] + y_val = y_line[len(LOGGING_PREFIX) + 1:-1] if numbers_are_close_enough(x_val, y_val): continue if x_line.startswith(FUZZ_EXEC_NOTE_RESULT) and y_line.startswith(FUZZ_EXEC_NOTE_RESULT): @@ -1844,6 +1844,220 @@ def get_relevant_lines(wat): compare(get_relevant_lines(original), get_relevant_lines(processed), 'Preserve') +# Test that we preserve branch hints properly. The invariant that we test here +# is that, given correct branch hints (that is, the input wasm's branch hints +# are always correct: a branch is taken iff the hint is that it is taken), then +# the optimizer does not end up with incorrect branch hints. It is fine if the +# optimizer removes some hints (it may remove entire chunks of code in DCE, for +# example, and it may find ways to simplify code so fewer things execute), but +# it should not emit a branch hint that is wrong - if it is not certain, it +# should remove the branch hint. +# +# Note that bugs found by this fuzzer tend to require the following during +# reducing: BINARYEN_TRUST_GIVEN_WASM=1 in the env, and --text as a parameter. +class BranchHintPreservation(TestCaseHandler): + frequency = 0.1 + + def handle(self, wasm): + # Generate an instrumented wasm. + instrumented = wasm + '.inst.wasm' + run([ + in_bin('wasm-opt'), + wasm, + '-o', instrumented, + # Add random branch hints (so we have something to work with). + '--randomize-branch-hints', + # Instrument them with logging. + '--instrument-branch-hints', + '-g', + ] + FEATURE_OPTS) + + # Collect the logging. + out = run_bynterp(instrumented, ['--fuzz-exec-before', '-all']) + + # Process the output. We look at the lines like this: + # + # [LoggingExternalInterface log-branch 1 0 0] + # + # where the three integers are: ID, predicted, actual. + all_ids = set() + bad_ids = set() + LOG_BRANCH_PREFIX = '[LoggingExternalInterface log-branch' + for line in out.splitlines(): + if line.startswith(LOG_BRANCH_PREFIX): + # (1:-1 strips away the '[', ']' at the edges) + _, _, id_, hint, actual = line[1:-1].split(' ') + all_ids.add(id_) + if hint != actual: + # This hint was misleading. + bad_ids.add(id_) + + # If no good ids remain, there is nothing to test (no hints will remain + # later down, after we remove bad ones). + if bad_ids == all_ids: + note_ignored_vm_run('no good ids') + return + + # Generate proper hints for testing: A wasm file with 100% valid branch + # hints, and instrumentation to verify that. + de_instrumented = wasm + '.de_inst.wasm' + args = [ + in_bin('wasm-opt'), + instrumented, + '-o', de_instrumented, + ] + # Remove the bad ids (using the instrumentation to identify them by ID). + if bad_ids: + args += [ + '--delete-branch-hints=' + ','.join(bad_ids), + ] + args += [ + # Remove all prior instrumentation, so it does not confuse us later + # when we log our final hints, and also so it does not inhibit + # optimizations. + '--deinstrument-branch-hints', + '-g', + ] + FEATURE_OPTS + run(args) + + # Add optimizations to see if things break. + opted = wasm + '.opted.wasm' + args = [ + in_bin('wasm-opt'), + de_instrumented, + '-o', opted, + '-g', + + # Some passes are just skipped, as they do not modify ifs or brs, + # but they do break the invariant of not adding bad branch hints. + # There are two main issues here: + # * Moving code around, possibly causing it to start to execute if + # it previously was not reached due to a trap (a branch hint + # seems to have no effects in the optimizer, so it will do such + # movements). And if it starts to execute and is a wrong hint, we + # get an invalid fuzzer finding. + # * LICM moves code out of loops. + '--skip-pass=licm', + # * HeapStoreOptimization moves struct.sets closer to struct.news. + '--skip-pass=heap-store-optimization', + # * MergeBlocks moves code out of inner blocks to outer blocks. + '--skip-pass=merge-blocks', + # * Monomorphize can subtly reorder code: + # + # (call $foo + # (select + # (i32.div_s ..which will trap..) + # (if with branch hint) + # => + # (call $foo_1 + # (if with branch hint) + # + # where $foo_1 receives the if's result and uses it in the + # ("reverse-inlined") select. Now the if executes first, when + # previously the trap stopped it. + '--skip-pass=monomorphize', + '--skip-pass=monomorphize-always', + # SimplifyGlobals finds globals that are "read only to be written", + # and can remove the ifs that do so: + # + # if (foo) { foo = 1 } + # => + # if (0) {} + # + # This is valid if the global's value is never read otherwise, but + # it does alter the if's behavior. + '--skip-pass=simplify-globals', + '--skip-pass=simplify-globals-optimizing', + + # * Merging/folding code. When we do so, code identical in content + # but differing in metadata will end up with the metadata from one + # of the copies, which might be wrong (we follow LLVM here, see + # details in the passes). + # * CodeFolding merges code blocks inside functions. + '--skip-pass=code-folding', + # * DuplicateFunctionElimination merges functions. + '--skip-pass=duplicate-function-elimination', + + # Some passes break the invariant in some cases, but we do not want + # to skip them entirely, as they have other things we need to fuzz. + # We add pass-args for them: + # * Do not fold inside OptimizeInstructions. + '--pass-arg=optimize-instructions-never-fold-or-reorder', + # * Do not unconditionalize code in RemoveUnusedBrs. + '--pass-arg=remove-unused-brs-never-unconditionalize', + + ] + get_random_opts() + FEATURE_OPTS + run(args) + + # Add instrumentation, to see if any branch hints are wrong after + # optimizations. We must do this in a separate invocation from the + # optimizations due to flags like --converge (which would instrument + # multiple times). + final = wasm + '.final.wasm' + args = [ + in_bin('wasm-opt'), + opted, + '-o', final, + '--instrument-branch-hints', + '-g', + ] + FEATURE_OPTS + run(args) + + # Run the final wasm. + out = run_bynterp(final, ['--fuzz-exec-before', '-all']) + + # Preprocess the logging. We must discard all lines from functions that + # trap, because we are fuzzing branch hints, which are not an effect, + # and so they can be reordered with traps; consider this: + # + # (i32.add + # (block + # (if (X) (unreachable) + # (i32.const 10) + # ) + # (block + # (@metadata.code.branch_hint "\00") + # (if (Y) (unreachable) + # (i32.const 20) + # ) + # ) + # + # It is ok to reorder traps, so the optimizer might flip the arms of + # this add (imagine other code inside the arms justified that). That + # reordering is fine since the branch hint has no effect that the + # optimizer needs to care about. However, after we instrument, there + # *is* an effect, the visible logging, so if X is true we trap and do + # not log a branch hint, but if we reorder, we do log, then trap. + # + # Note that this problem is specific to traps, because the optimizer can + # reorder them, and does not care about identity. + # + # To handle this, gather lines for each call, and then see which groups + # end in traps. (Initialize the list of groups with an empty group, for + # any logging before the first call.) + line_groups = [['before calls']] + for line in out.splitlines(): + if line.startswith(FUZZ_EXEC_CALL_PREFIX): + line_groups.append([line]) + else: + line_groups[-1].append(line) + + # No bad hints should pop up after optimizations. + for group in line_groups: + if not group or group[-1] == '[trap unreachable]': + continue + for line in group: + if line.startswith(LOG_BRANCH_PREFIX): + _, _, id_, hint, actual = line[1:-1].split(' ') + hint = int(hint) + actual = int(actual) + assert hint in (0, 1) + # We do not care about the integer value of the condition, + # only if it was 0 or non-zero. + actual = (actual != 0) + assert hint == actual, 'Bad hint after optimizations' + + # The global list of all test case handlers testcase_handlers = [ FuzzExec(), @@ -1859,6 +2073,7 @@ def get_relevant_lines(wat): ClusterFuzz(), Two(), PreserveImportsExports(), + BranchHintPreservation(), ] diff --git a/scripts/fuzz_shell.js b/scripts/fuzz_shell.js index 3f201b3c812..ba853a6e538 100644 --- a/scripts/fuzz_shell.js +++ b/scripts/fuzz_shell.js @@ -353,6 +353,10 @@ var imports = { // how many time units to wait). }); }, + + 'log-branch': (id, expected, actual) => { + console.log(`[LoggingExternalInterface log-branch ${id} ${expected} ${actual}]`); + }, }, // Emscripten support. 'env': { diff --git a/src/passes/CodeFolding.cpp b/src/passes/CodeFolding.cpp index 305eb12784f..ee30d3e5080 100644 --- a/src/passes/CodeFolding.cpp +++ b/src/passes/CodeFolding.cpp @@ -249,6 +249,10 @@ struct CodeFolding // run the rest of the optimization mormally. auto maybeAddBlock = [this](Block* block, Expression*& other) -> Block* { // If other is a suffix of the block, wrap it in a block. + // + // Note that we do not consider metadata here. Like LLVM, we ignore + // metadata when trying to fold code together, preferring certain + // optimization over possible benefits of profiling data. if (block->list.empty() || !ExpressionAnalyzer::equal(other, block->list.back())) { return nullptr; diff --git a/src/passes/InstrumentBranchHints.cpp b/src/passes/InstrumentBranchHints.cpp index 35d2dbabb15..34af0953662 100644 --- a/src/passes/InstrumentBranchHints.cpp +++ b/src/passes/InstrumentBranchHints.cpp @@ -54,11 +54,56 @@ // if (expected != actual) throw `Bad branch hint! (${id})`; // }; // +// A pass to delete branch hints is also provided, which finds instrumentations +// and the IDs in those calls, and deletes branch hints that were listed. For +// example, +// +// --delete-branch-hints=10,20 +// +// would do this transformation: +// +// @metadata.branch.hint A +// if (temp = condition; log(10, A, temp); temp) { // 10 matches one of 10,20 +// X +// } +// @metadata.branch.hint B +// if (temp = condition; log(99, B, temp); temp) { // 99 does not match +// Y +// } +// +// => +// +// // Used to be a branch hint here, but it was deleted. +// if (temp = condition; log(10, A, temp); temp) { +// X +// } +// @metadata.branch.hint B // this one is unmodified. +// if (temp = condition; log(99, B, temp); temp) { +// Y +// } +// +// A pass to undo the instrumentation is also provided, which does +// +// if (temp = condition; log(123, A, temp); temp) { +// X +// } +// +// => +// +// if (condition) { +// X +// } +// +#include "ir/drop.h" #include "ir/eh-utils.h" +#include "ir/find_all.h" +#include "ir/local-graph.h" #include "ir/names.h" +#include "ir/parents.h" #include "ir/properties.h" #include "pass.h" +#include "support/string.h" #include "wasm-builder.h" #include "wasm.h" @@ -66,6 +111,20 @@ namespace wasm { namespace { +// The module and base names of our import. +const Name MODULE = "fuzzing-support"; +const Name BASE = "log-branch"; + +// Finds our import, if it exists. +Name getLogBranchImport(Module* module) { + for (auto& func : module->functions) { + if (func->module == MODULE && func->base == BASE) { + return func->name; + } + } + return Name(); +} + // The branch id, which increments as we go. int branchId = 1; @@ -74,10 +133,6 @@ struct InstrumentBranchHints using Super = WalkerPass>; - // The module and base names of our import. - const Name MODULE = "fuzzing-support"; - const Name BASE = "log-branch"; - // The internal name of our import. Name logBranch; @@ -132,31 +187,222 @@ struct InstrumentBranchHints } void doWalkModule(Module* module) { - // Find our import, if we were already run on this module. - for (auto& func : module->functions) { - if (func->module == MODULE && func->base == BASE) { - logBranch = func->name; - break; - } + if (auto existing = getLogBranchImport(module)) { + // This file already has our import. We nop it out, as whatever the + // current code does may be dangerous (it may log incorrect hints). + auto* func = module->getFunction(existing); + func->body = Builder(*module).makeNop(); + func->module = func->base = Name(); + } + + // Add our import. + auto* func = module->addFunction(Builder::makeFunction( + Names::getValidFunctionName(*module, BASE), + Signature({Type::i32, Type::i32, Type::i32}, Type::none), + {})); + func->module = MODULE; + func->base = BASE; + logBranch = func->name; + + // Walk normally, using logBranch as we go. + Super::doWalkModule(module); + } +}; + +// Helper class that provides basic utilities for identifying and processing +// instrumentation from InstrumentBranchHints. +template +struct InstrumentationProcessor : public WalkerPass> { + + using Super = WalkerPass>; + + // The internal name of our import. + Name logBranch; + + // A LocalGraph, so we can identify the pattern. + std::unique_ptr localGraph; + + // A map of expressions to their parents, so we can identify the pattern. + std::unique_ptr parents; + + Sub* self() { return static_cast(this); } + + void visitIf(If* curr) { self()->processCondition(curr); } + + void visitBreak(Break* curr) { + if (curr->condition) { + self()->processCondition(curr); } - // Otherwise, add it. + } + + // TODO: BrOn, but the condition there is not an i32 + + void doWalkFunction(Function* func) { + localGraph = std::make_unique(func, this->getModule()); + localGraph->computeSetInfluences(); + + parents = std::make_unique(func->body); + + Super::doWalkFunction(func); + } + + void doWalkModule(Module* module) { + logBranch = getLogBranchImport(module); if (!logBranch) { - auto* func = module->addFunction(Builder::makeFunction( - Names::getValidFunctionName(*module, BASE), - Signature({Type::i32, Type::i32, Type::i32}, Type::none), - {})); - func->module = MODULE; - func->base = BASE; - logBranch = func->name; + Fatal() + << "No branch hint logging import found. Was this code instrumented?"; } - // Walk normally, using logBranch as we go. Super::doWalkModule(module); } + + // Helpers + + // Instrumentation info for a chunk of code that is the result of the + // instrumentation pass. + struct Instrumentation { + // The condition before the instrumentation (a pointer to it, so we can + // replace it). + Expression** originalCondition; + // The call to the logging that the instrumentation added. + Call* call; + }; + + // Check if an expression's condition is an instrumentation, and return the + // info if so. + std::optional getInstrumentation(Expression* condition) { + // We must identify this pattern: + // + // (br_if + // (block + // (local.set $temp (condition)) + // (call $log (id, prediction, (local.get $temp))) + // (local.get $temp) + // ) + // + // The block may vanish during roundtrip though, so we just follow back from + // the last local.get, which appears in the condition: + // + // (local.set $temp (condition)) + // (call $log (id, prediction, (local.get $temp))) + // (br_if + // (local.get $temp) + // + auto* fallthrough = Properties::getFallthrough( + condition, this->getPassOptions(), *this->getModule()); + auto* get = fallthrough->template dynCast(); + if (!get) { + return {}; + } + auto& sets = localGraph->getSets(get); + if (sets.size() != 1) { + return {}; + } + auto* set = *sets.begin(); + if (!set) { + return {}; + } + auto& gets = localGraph->getSetInfluences(set); + if (gets.size() != 2) { + return {}; + } + // The set has two gets: the get in the condition we began at, and + // another. + LocalGet* otherGet = nullptr; + for (auto* get2 : gets) { + if (get2 != get) { + otherGet = get2; + } + } + assert(otherGet); + // See if that other get is used in a logging. The parent should be a + // logging call. + auto* call = parents->getParent(otherGet)->template dynCast(); + if (!call || call->target != logBranch) { + return {}; + } + // Great, this is indeed a prior instrumentation. + return Instrumentation{&set->value, call}; + } +}; + +struct DeleteBranchHints : public InstrumentationProcessor { + using Super = InstrumentationProcessor; + + // The set of IDs to delete. + std::unordered_set idsToDelete; + + template void processCondition(T* curr) { + if (auto info = getInstrumentation(curr->condition)) { + if (auto* c = info->call->operands[0]->template dynCast()) { + auto id = c->value.geti32(); + if (idsToDelete.count(id)) { + // Remove the branch hint. + getFunction()->codeAnnotations[curr].branchLikely = {}; + } + } + } + } + + void doWalkModule(Module* module) { + auto arg = getArgument( + "delete-branch-hints", + "DeleteBranchHints usage: wasm-opt --delete-branch-hints=10,20,30"); + for (auto& str : String::Split(arg, String::Split::NewLineOr(","))) { + idsToDelete.insert(std::stoi(str)); + } + + Super::doWalkModule(module); + } +}; + +struct DeInstrumentBranchHints + : public InstrumentationProcessor { + + template void processCondition(T* curr) { + if (auto info = getInstrumentation(curr->condition)) { + // Replace the instrumented condition with the original one (swap so that + // the IR remains valid: we cannot use the same expression twice in our + // IR, and the original condition is still used in another place, until + // we remove the logging calls; since we will remove the calls anyhow, we + // just need some valid IR there). + std::swap(curr->condition, *info->originalCondition); + } + } + + void visitFunction(Function* func) { + if (func->imported()) { + return; + } + // At the very end, remove all logging calls (we use them during the main + // walk to identify instrumentation). + for (auto** callp : FindAllPointers(func->body).list) { + auto* call = (*callp)->cast(); + if (call->target == logBranch) { + Builder builder(*getModule()); + Expression* last; + if (call->type == Type::none) { + last = builder.makeNop(); + } else { + last = builder.makeUnreachable(); + } + *callp = getDroppedChildrenAndAppend(call, + *getModule(), + getPassOptions(), + last, + // We know the call is removable. + DropMode::IgnoreParentEffects); + } + } + } }; } // anonymous namespace Pass* createInstrumentBranchHintsPass() { return new InstrumentBranchHints(); } +Pass* createDeleteBranchHintsPass() { return new DeleteBranchHints(); } +Pass* createDeInstrumentBranchHintsPass() { + return new DeInstrumentBranchHints(); +} } // namespace wasm diff --git a/src/passes/LocalCSE.cpp b/src/passes/LocalCSE.cpp index 78f722c7ae5..9c9a8198f89 100644 --- a/src/passes/LocalCSE.cpp +++ b/src/passes/LocalCSE.cpp @@ -157,6 +157,11 @@ struct HEComparer { if (a.digest != b.digest) { return false; } + // Note that we do not consider metadata here. That means we may replace two + // identical expressions with different metadata, say, different branch + // hints, but that is ok: we are only removing things from executing (by + // reusing the first computed value), so this will not cause new invalid + // branch hints to execute. return ExpressionAnalyzer::equal(a.expr, b.expr); } }; diff --git a/src/passes/OptimizeInstructions.cpp b/src/passes/OptimizeInstructions.cpp index 9ad27dc67ab..b2b937dd2a3 100644 --- a/src/passes/OptimizeInstructions.cpp +++ b/src/passes/OptimizeInstructions.cpp @@ -228,11 +228,25 @@ struct OptimizeInstructions bool fastMath; + // If set, we never fold/merge code together. This is important when fuzzing + // branch hints, as if we allow folding, then we may fold code identical in + // all ways but for branch hints, leading to an invalid branch hint executing + // later (imagine one arm had the right hint and the other the wrong one; we + // leave one of the two arbitrarily, so we might get unlucky). + bool neverFold; + + // As neverFold, but for reordering code. If we move a branch hint around code + // that might trap, and the trap happens later, the branch hint might start to + // execute, and it could be wrong. + bool neverReorder; + // In rare cases we make a change to a type, and will do a refinalize. bool refinalize = false; void doWalkFunction(Function* func) { fastMath = getPassOptions().fastMath; + neverFold = neverReorder = + hasArgument("optimize-instructions-never-fold-or-reorder"); // First, scan locals. { @@ -312,6 +326,9 @@ struct OptimizeInstructions } bool canReorder(Expression* a, Expression* b) { + if (neverReorder) { + return false; + } return EffectAnalyzer::canReorder(getPassOptions(), *getModule(), a, b); } @@ -1176,7 +1193,10 @@ struct OptimizeInstructions BranchHints::flip(curr, getFunction()); } } - if (curr->condition->type != Type::unreachable && + // Note that we do not consider metadata here. Like LLVM, we ignore + // metadata when trying to fold code together, preferring certain + // optimization over possible benefits of profiling data. + if (!neverFold && curr->condition->type != Type::unreachable && ExpressionAnalyzer::equal(curr->ifTrue, curr->ifFalse)) { // The sides are identical, so fold. If we can replace the If with one // arm and there are no side effects in the condition, replace it. But @@ -2820,6 +2840,9 @@ struct OptimizeInstructions // write more concise pattern matching code elsewhere. void canonicalize(Binary* binary) { assert(shouldCanonicalize(binary)); + if (neverReorder) { + return; + } auto swap = [&]() { assert(canReorder(binary->left, binary->right)); if (binary->isRelational()) { @@ -3235,8 +3258,12 @@ struct OptimizeInstructions } } { - // Sides are identical, fold + // If sides are identical, fold. Expression *ifTrue, *ifFalse, *c; + // Note we do not compare metadata here: This is a select, so both arms + // execute anyhow, and things like branch hints were already being run. + // After optimization, we will only run fewer things, and run no risk of + // running new bad things. if (matches(curr, select(any(&ifTrue), any(&ifFalse), any(&c))) && ExpressionAnalyzer::equal(ifTrue, ifFalse)) { auto value = effects(ifTrue); @@ -5642,7 +5669,7 @@ struct OptimizeInstructions } } - { + if (!neverFold) { // Identical code on both arms can be folded out, e.g. // // (select diff --git a/src/passes/RandomizeBranchHints.cpp b/src/passes/RandomizeBranchHints.cpp index 7b013fc7ebf..b74d2e8a093 100644 --- a/src/passes/RandomizeBranchHints.cpp +++ b/src/passes/RandomizeBranchHints.cpp @@ -50,6 +50,8 @@ struct RandomizeBranchHints } } + // TODO: BrOn + template void processCondition(T* curr) { auto& likely = getFunction()->codeAnnotations[curr].branchLikely; switch (hash % 3) { diff --git a/src/passes/RemoveUnusedBrs.cpp b/src/passes/RemoveUnusedBrs.cpp index b2237dcb823..4ea8592c356 100644 --- a/src/passes/RemoveUnusedBrs.cpp +++ b/src/passes/RemoveUnusedBrs.cpp @@ -15,7 +15,7 @@ */ // -// Removes branches for which we go to where they go anyhow +// Removes branches for which we go to where they go anyhow. // #include "ir/branch-hints.h" @@ -161,6 +161,12 @@ struct RemoveUnusedBrs : public WalkerPass> { bool anotherCycle; + // Whether we are allowed to unconditionalize code, that is, make code run + // that previously might not have. Unconditionalizing code is a problem for + // fuzzing branch hints: a branch hint that never ran might be wrong, and if + // we start to run it, the fuzzer would report a finding. + bool neverUnconditionalize; + using Flows = std::vector; // list of breaks that are currently flowing. if they reach their target @@ -408,7 +414,12 @@ struct RemoveUnusedBrs : public WalkerPass> { // zero (also 3 bytes). The size is unchanged, but the select may // be further optimizable, and if select does not branch we also // avoid one branch. - // Multivalue selects are not supported + if (neverUnconditionalize) { + // Creating a select, below, would unconditionally run the + // select's condition. + return; + } + // Multivalue selects are not supported. if (br->value && br->value->type.isTuple()) { return; } @@ -449,6 +460,11 @@ struct RemoveUnusedBrs : public WalkerPass> { if (child->ifFalse) { return; } + if (neverUnconditionalize) { + // Creating a select, below, would unconditionally run the inner if's + // condition (condition-B, in the comment above). + return; + } // If running the child's condition unconditionally is too expensive, // give up. if (tooCostlyToRunUnconditionally(getPassOptions(), child->condition)) { @@ -1129,6 +1145,9 @@ struct RemoveUnusedBrs : public WalkerPass> { } void doWalkFunction(Function* func) { + neverUnconditionalize = + hasArgument("remove-unused-brs-never-unconditionalize"); + // multiple cycles may be needed do { anotherCycle = false; @@ -1252,9 +1271,11 @@ struct RemoveUnusedBrs : public WalkerPass> { // perform some final optimizations struct FinalOptimizer : public PostWalker { - bool shrink; PassOptions& passOptions; + bool shrink; + bool neverUnconditionalize; + bool needUniqify = false; bool refinalize = false; @@ -1520,6 +1541,11 @@ struct RemoveUnusedBrs : public WalkerPass> { // must not have side effects. // TODO: we can do this when there *are* other refs to $x, // with a larger refactoring here. + if (neverUnconditionalize) { + // If we optimize, we'd unconditionally execute the rest of + // the block. + return; + } // Test for the conditions with a temporary nop instead of the // br_if. @@ -1556,6 +1582,9 @@ struct RemoveUnusedBrs : public WalkerPass> { // Convert an if into a select, if possible and beneficial to do so. Select* selectify(If* iff) { + if (neverUnconditionalize) { + return nullptr; + } // Only an if-else can be turned into a select. if (!iff->ifFalse) { return nullptr; @@ -2011,6 +2040,8 @@ struct RemoveUnusedBrs : public WalkerPass> { FinalOptimizer finalOptimizer(getPassOptions()); finalOptimizer.setModule(getModule()); finalOptimizer.shrink = getPassRunner()->options.shrinkLevel > 0; + finalOptimizer.neverUnconditionalize = neverUnconditionalize; + finalOptimizer.walkFunction(func); if (finalOptimizer.needUniqify) { wasm::UniqueNameMapper::uniquify(func->body); diff --git a/src/passes/pass.cpp b/src/passes/pass.cpp index e0b7595b0b5..8db59b8ac0f 100644 --- a/src/passes/pass.cpp +++ b/src/passes/pass.cpp @@ -412,9 +412,6 @@ void PassRegistry::registerPasses() { registerPass("propagate-globals-globally", "propagate global values to other globals (useful for tests)", createPropagateGlobalsGloballyPass); - registerTestPass("randomize-branch-hints", - "randomize branch hints (for fuzzing)", - createRandomizeBranchHintsPass); registerPass("remove-non-js-ops", "removes operations incompatible with js", createRemoveNonJSOpsPass); @@ -451,9 +448,6 @@ void PassRegistry::registerPasses() { registerPass("reorder-globals", "sorts globals by access frequency", createReorderGlobalsPass); - registerTestPass("reorder-globals-always", - "sorts globals by access frequency (even if there are few)", - createReorderGlobalsAlwaysPass); registerPass("reorder-locals", "sorts locals by access frequency", createReorderLocalsPass); @@ -599,9 +593,21 @@ void PassRegistry::registerPasses() { registerTestPass("catch-pop-fixup", "fixup nested pops within catches", createCatchPopFixupPass); + registerTestPass("deinstrument-branch-hints", + "de-instrument branch hint instrumentation", + createDeInstrumentBranchHintsPass); + registerTestPass("delete-branch-hints", + "delete branch hints using a list of instrumented IDs", + createDeleteBranchHintsPass); registerTestPass("experimental-type-generalizing", "generalize types (not yet sound)", createTypeGeneralizingPass); + registerTestPass("randomize-branch-hints", + "randomize branch hints (for fuzzing)", + createRandomizeBranchHintsPass); + registerTestPass("reorder-globals-always", + "sorts globals by access frequency (even if there are few)", + createReorderGlobalsAlwaysPass); } void PassRunner::addIfNoDWARFIssues(std::string passName) { diff --git a/src/passes/passes.h b/src/passes/passes.h index e0c03bad8d7..92dcd3e4eb4 100644 --- a/src/passes/passes.h +++ b/src/passes/passes.h @@ -37,6 +37,8 @@ Pass* createDAEPass(); Pass* createDAEOptimizingPass(); Pass* createDataFlowOptsPass(); Pass* createDeadCodeEliminationPass(); +Pass* createDeInstrumentBranchHintsPass(); +Pass* createDeleteBranchHintsPass(); Pass* createDeNaNPass(); Pass* createDeAlignPass(); Pass* createDebugLocationPropagationPass(); diff --git a/src/tools/execution-results.h b/src/tools/execution-results.h index 0dea839c839..03e9ccb47aa 100644 --- a/src/tools/execution-results.h +++ b/src/tools/execution-results.h @@ -79,7 +79,15 @@ struct LoggingExternalInterface : public ShellExternalInterface { if (import->module == "fuzzing-support") { if (import->base.startsWith("log")) { // This is a logging function like log-i32 or log-f64 - std::cout << "[LoggingExternalInterface logging"; + std::cout << "[LoggingExternalInterface "; + if (import->base == "log-branch") { + // Report this as a special logging, so we can differentiate it from + // the others in the fuzzer. + std::cout << "log-branch"; + } else { + // All others are just reported as loggings. + std::cout << "logging"; + } loggings.push_back(Literal()); // buffer with a None between calls for (auto argument : arguments) { if (argument.type == Type::i64) { diff --git a/test/lit/passes/code-folding_branch-hints.wast b/test/lit/passes/code-folding_branch-hints.wast new file mode 100644 index 00000000000..51601230865 --- /dev/null +++ b/test/lit/passes/code-folding_branch-hints.wast @@ -0,0 +1,141 @@ +;; NOTE: Assertions have been generated by update_lit_checks.py --all-items and should not be edited. +;; NOTE: This test was ported using port_passes_tests_to_lit.py and could be cleaned up. + +;; RUN: wasm-opt %s -all --code-folding -S -o - | filecheck %s + +(module + ;; CHECK: (type $0 (func (param i32 i32) (result f32))) + + ;; CHECK: (func $different (type $0) (param $x i32) (param $y i32) (result f32) + ;; CHECK-NEXT: (drop + ;; CHECK-NEXT: (local.get $x) + ;; CHECK-NEXT: ) + ;; CHECK-NEXT: (@metadata.code.branch_hint "\00") + ;; CHECK-NEXT: (if + ;; CHECK-NEXT: (local.get $x) + ;; CHECK-NEXT: (then + ;; CHECK-NEXT: (nop) + ;; CHECK-NEXT: ) + ;; CHECK-NEXT: ) + ;; CHECK-NEXT: (f32.const 0) + ;; CHECK-NEXT: ) + (func $different (param $x i32) (param $y i32) (result f32) + ;; The branch hints differ, but we still optimize (like LLVM). + (if (result f32) + (local.get $x) + (then + (block (result f32) + (@metadata.code.branch_hint "\00") + (if + (local.get $x) + (then + (nop) + ) + ) + (f32.const 0) + ) + ) + (else + (block (result f32) + (@metadata.code.branch_hint "\01") + (if + (local.get $x) + (then + (nop) + ) + ) + (f32.const 0) + ) + ) + ) + ) + + ;; CHECK: (func $different-flip (type $0) (param $x i32) (param $y i32) (result f32) + ;; CHECK-NEXT: (drop + ;; CHECK-NEXT: (local.get $x) + ;; CHECK-NEXT: ) + ;; CHECK-NEXT: (@metadata.code.branch_hint "\01") + ;; CHECK-NEXT: (if + ;; CHECK-NEXT: (local.get $x) + ;; CHECK-NEXT: (then + ;; CHECK-NEXT: (nop) + ;; CHECK-NEXT: ) + ;; CHECK-NEXT: ) + ;; CHECK-NEXT: (f32.const 0) + ;; CHECK-NEXT: ) + (func $different-flip (param $x i32) (param $y i32) (result f32) + ;; As above, but flipped. We still optimize, still keeping the first branch + ;; hint (now "\01"). + (if (result f32) + (local.get $x) + (then + (block (result f32) + (@metadata.code.branch_hint "\01") + (if + (local.get $x) + (then + (nop) + ) + ) + (f32.const 0) + ) + ) + (else + (block (result f32) + (@metadata.code.branch_hint "\00") + (if + (local.get $x) + (then + (nop) + ) + ) + (f32.const 0) + ) + ) + ) + ) + + ;; CHECK: (func $same (type $0) (param $x i32) (param $y i32) (result f32) + ;; CHECK-NEXT: (drop + ;; CHECK-NEXT: (local.get $x) + ;; CHECK-NEXT: ) + ;; CHECK-NEXT: (@metadata.code.branch_hint "\00") + ;; CHECK-NEXT: (if + ;; CHECK-NEXT: (local.get $x) + ;; CHECK-NEXT: (then + ;; CHECK-NEXT: (nop) + ;; CHECK-NEXT: ) + ;; CHECK-NEXT: ) + ;; CHECK-NEXT: (f32.const 0) + ;; CHECK-NEXT: ) + (func $same (param $x i32) (param $y i32) (result f32) + ;; The branch hints are the same, so we definitely optimize. + (if (result f32) + (local.get $x) + (then + (block (result f32) + (@metadata.code.branch_hint "\00") + (if + (local.get $x) + (then + (nop) + ) + ) + (f32.const 0) + ) + ) + (else + (block (result f32) + (@metadata.code.branch_hint "\00") + (if + (local.get $x) + (then + (nop) + ) + ) + (f32.const 0) + ) + ) + ) + ) +) diff --git a/test/lit/passes/deinstrument-branch-hints.wast b/test/lit/passes/deinstrument-branch-hints.wast new file mode 100644 index 00000000000..6d619c4b28b --- /dev/null +++ b/test/lit/passes/deinstrument-branch-hints.wast @@ -0,0 +1,167 @@ +;; NOTE: Assertions have been generated by update_lit_checks.py --all-items and should not be edited. + +;; RUN: foreach %s %t wasm-opt -all --deinstrument-branch-hints -S -o - | filecheck %s + +(module + ;; CHECK: (type $0 (func)) + + ;; CHECK: (type $1 (func (param i32 i32 i32))) + + ;; CHECK: (import "fuzzing-support" "log-branch" (func $log (type $1) (param i32 i32 i32))) + (import "fuzzing-support" "log-branch" (func $log (param i32 i32 i32))) + + ;; CHECK: (func $if (type $0) + ;; CHECK-NEXT: (local $temp i32) + ;; CHECK-NEXT: (@metadata.code.branch_hint "\00") + ;; CHECK-NEXT: (if + ;; CHECK-NEXT: (i32.const 42) + ;; CHECK-NEXT: (then + ;; CHECK-NEXT: (drop + ;; CHECK-NEXT: (i32.const 1337) + ;; CHECK-NEXT: ) + ;; CHECK-NEXT: ) + ;; CHECK-NEXT: (else + ;; CHECK-NEXT: (drop + ;; CHECK-NEXT: (i32.const 99) + ;; CHECK-NEXT: ) + ;; CHECK-NEXT: ) + ;; CHECK-NEXT: ) + ;; CHECK-NEXT: ) + (func $if + (local $temp i32) + ;; The instrumentation should be removed, and the if's condition should + ;; be 42. + (@metadata.code.branch_hint "\00") + (if + (block (result i32) + (local.set $temp + (i32.const 42) + ) + (call $log + (i32.const 1) + (i32.const 0) + (local.get $temp) + ) + (local.get $temp) + ) + (then + (drop + (i32.const 1337) + ) + ) + (else + (drop + (i32.const 99) + ) + ) + ) + ) + + ;; CHECK: (func $br (type $0) + ;; CHECK-NEXT: (local $temp i32) + ;; CHECK-NEXT: (block $out + ;; CHECK-NEXT: (@metadata.code.branch_hint "\01") + ;; CHECK-NEXT: (br_if $out + ;; CHECK-NEXT: (i32.const 42) + ;; CHECK-NEXT: ) + ;; CHECK-NEXT: ) + ;; CHECK-NEXT: ) + (func $br + ;; The same, with a br. + (local $temp i32) + (block $out + (@metadata.code.branch_hint "\01") + (br_if $out + (block (result i32) + (local.set $temp + (i32.const 42) + ) + (call $log + (i32.const 4) + (i32.const 0) + (local.get $temp) + ) + (local.get $temp) + ) + ) + ) + ) + + ;; CHECK: (func $br-before (type $0) + ;; CHECK-NEXT: (local $temp i32) + ;; CHECK-NEXT: (block $out + ;; CHECK-NEXT: (local.set $temp + ;; CHECK-NEXT: (local.get $temp) + ;; CHECK-NEXT: ) + ;; CHECK-NEXT: (nop) + ;; CHECK-NEXT: (@metadata.code.branch_hint "\01") + ;; CHECK-NEXT: (br_if $out + ;; CHECK-NEXT: (i32.const 42) + ;; CHECK-NEXT: ) + ;; CHECK-NEXT: ) + ;; CHECK-NEXT: ) + (func $br-before + ;; As above, but the instrumentation is before us, leaving only a local.get + ;; in the br's condition. We should still identify the pattern and remove + ;; the logging (but we leave the local.set for other things to clean up). + (local $temp i32) + (block $out + (local.set $temp + (i32.const 42) + ) + (call $log + (i32.const 4) + (i32.const 0) + (local.get $temp) + ) + (@metadata.code.branch_hint "\01") + (br_if $out + (local.get $temp) + ) + ) + ) + + ;; CHECK: (func $br-before-effects (type $0) + ;; CHECK-NEXT: (local $temp i32) + ;; CHECK-NEXT: (local $other i32) + ;; CHECK-NEXT: (block $out + ;; CHECK-NEXT: (local.set $temp + ;; CHECK-NEXT: (local.get $temp) + ;; CHECK-NEXT: ) + ;; CHECK-NEXT: (block + ;; CHECK-NEXT: (drop + ;; CHECK-NEXT: (local.tee $other + ;; CHECK-NEXT: (i32.const 0) + ;; CHECK-NEXT: ) + ;; CHECK-NEXT: ) + ;; CHECK-NEXT: (nop) + ;; CHECK-NEXT: ) + ;; CHECK-NEXT: (@metadata.code.branch_hint "\01") + ;; CHECK-NEXT: (br_if $out + ;; CHECK-NEXT: (i32.const 42) + ;; CHECK-NEXT: ) + ;; CHECK-NEXT: ) + ;; CHECK-NEXT: ) + (func $br-before-effects + ;; As above, but there are effects in the call's children that we must + ;; keep. + (local $temp i32) + (local $other i32) + (block $out + (local.set $temp + (i32.const 42) + ) + (call $log + (i32.const 4) + (local.tee $other ;; this tee must be kept around + (i32.const 0) + ) + (local.get $temp) + ) + (@metadata.code.branch_hint "\01") + (br_if $out + (local.get $temp) + ) + ) + ) +) diff --git a/test/lit/passes/delete-branch-hints.wast b/test/lit/passes/delete-branch-hints.wast new file mode 100644 index 00000000000..375b10d16c2 --- /dev/null +++ b/test/lit/passes/delete-branch-hints.wast @@ -0,0 +1,163 @@ +;; NOTE: Assertions have been generated by update_lit_checks.py --all-items and should not be edited. + +;; RUN: foreach %s %t wasm-opt -all --delete-branch-hints=10,30 -S -o - | filecheck %s + +(module + ;; CHECK: (type $0 (func)) + + ;; CHECK: (type $1 (func (param i32 i32 i32))) + + ;; CHECK: (import "fuzzing-support" "log-branch" (func $log (type $1) (param i32 i32 i32))) + (import "fuzzing-support" "log-branch" (func $log (param i32 i32 i32))) + + ;; CHECK: (func $if-10 (type $0) + ;; CHECK-NEXT: (local $temp i32) + ;; CHECK-NEXT: (if + ;; CHECK-NEXT: (block (result i32) + ;; CHECK-NEXT: (local.set $temp + ;; CHECK-NEXT: (i32.const 42) + ;; CHECK-NEXT: ) + ;; CHECK-NEXT: (call $log + ;; CHECK-NEXT: (i32.const 10) + ;; CHECK-NEXT: (i32.const 0) + ;; CHECK-NEXT: (local.get $temp) + ;; CHECK-NEXT: ) + ;; CHECK-NEXT: (local.get $temp) + ;; CHECK-NEXT: ) + ;; CHECK-NEXT: (then + ;; CHECK-NEXT: (drop + ;; CHECK-NEXT: (i32.const 1337) + ;; CHECK-NEXT: ) + ;; CHECK-NEXT: ) + ;; CHECK-NEXT: (else + ;; CHECK-NEXT: (drop + ;; CHECK-NEXT: (i32.const 99) + ;; CHECK-NEXT: ) + ;; CHECK-NEXT: ) + ;; CHECK-NEXT: ) + ;; CHECK-NEXT: ) + (func $if-10 + (local $temp i32) + ;; The branch hint should be removed, since the ID "10" is in the list of + ;; 10, 30. + (@metadata.code.branch_hint "\00") + (if + (block (result i32) + (local.set $temp + (i32.const 42) + ) + (call $log + (i32.const 10) + (i32.const 0) + (local.get $temp) + ) + (local.get $temp) + ) + (then + (drop + (i32.const 1337) + ) + ) + (else + (drop + (i32.const 99) + ) + ) + ) + ) + + ;; CHECK: (func $if-20 (type $0) + ;; CHECK-NEXT: (local $temp i32) + ;; CHECK-NEXT: (@metadata.code.branch_hint "\00") + ;; CHECK-NEXT: (if + ;; CHECK-NEXT: (block (result i32) + ;; CHECK-NEXT: (local.set $temp + ;; CHECK-NEXT: (i32.const 42) + ;; CHECK-NEXT: ) + ;; CHECK-NEXT: (call $log + ;; CHECK-NEXT: (i32.const 20) + ;; CHECK-NEXT: (i32.const 0) + ;; CHECK-NEXT: (local.get $temp) + ;; CHECK-NEXT: ) + ;; CHECK-NEXT: (local.get $temp) + ;; CHECK-NEXT: ) + ;; CHECK-NEXT: (then + ;; CHECK-NEXT: (drop + ;; CHECK-NEXT: (i32.const 1337) + ;; CHECK-NEXT: ) + ;; CHECK-NEXT: ) + ;; CHECK-NEXT: (else + ;; CHECK-NEXT: (drop + ;; CHECK-NEXT: (i32.const 99) + ;; CHECK-NEXT: ) + ;; CHECK-NEXT: ) + ;; CHECK-NEXT: ) + ;; CHECK-NEXT: ) + (func $if-20 + (local $temp i32) + ;; The branch hint should *not* be removed: 20 is not in the list. + (@metadata.code.branch_hint "\00") + (if + (block (result i32) + (local.set $temp + (i32.const 42) + ) + (call $log + (i32.const 20) + (i32.const 0) + (local.get $temp) + ) + (local.get $temp) + ) + (then + (drop + (i32.const 1337) + ) + ) + (else + (drop + (i32.const 99) + ) + ) + ) + ) + + ;; CHECK: (func $br-30 (type $0) + ;; CHECK-NEXT: (local $temp i32) + ;; CHECK-NEXT: (block $out + ;; CHECK-NEXT: (br_if $out + ;; CHECK-NEXT: (block (result i32) + ;; CHECK-NEXT: (local.set $temp + ;; CHECK-NEXT: (i32.const 42) + ;; CHECK-NEXT: ) + ;; CHECK-NEXT: (call $log + ;; CHECK-NEXT: (i32.const 30) + ;; CHECK-NEXT: (i32.const 0) + ;; CHECK-NEXT: (local.get $temp) + ;; CHECK-NEXT: ) + ;; CHECK-NEXT: (local.get $temp) + ;; CHECK-NEXT: ) + ;; CHECK-NEXT: ) + ;; CHECK-NEXT: ) + ;; CHECK-NEXT: ) + (func $br-30 + ;; The hint should be removed. + (local $temp i32) + (block $out + (@metadata.code.branch_hint "\01") + (br_if $out + (block (result i32) + (local.set $temp + (i32.const 42) + ) + (call $log + (i32.const 30) + (i32.const 0) + (local.get $temp) + ) + (local.get $temp) + ) + ) + ) + ) +) diff --git a/test/lit/passes/instrument-branch-hints.wast b/test/lit/passes/instrument-branch-hints.wast index 565bcf78716..5a6d73f5e0e 100644 --- a/test/lit/passes/instrument-branch-hints.wast +++ b/test/lit/passes/instrument-branch-hints.wast @@ -433,15 +433,20 @@ ) ) -;; This module has our import, but with a minified internal name. We should use -;; that import. +;; This module has an existing import with our module and base names. We nop it +;; and create a fresh one, to avoid confusion. (module + (import "fuzzing-support" "log-branch" (func $existing (param i32 i32 i32))) + ;; CHECK: (type $0 (func (param i32 i32 i32))) ;; CHECK: (type $1 (func)) - ;; CHECK: (import "fuzzing-support" "log-branch" (func $min (type $0) (param i32 i32 i32))) - (import "fuzzing-support" "log-branch" (func $min (param i32 i32 i32))) + ;; CHECK: (import "fuzzing-support" "log-branch" (func $log-branch (type $0) (param i32 i32 i32))) + + ;; CHECK: (func $existing (type $0) (param $0 i32) (param $1 i32) (param $2 i32) + ;; CHECK-NEXT: (nop) + ;; CHECK-NEXT: ) ;; CHECK: (func $if (type $1) ;; CHECK-NEXT: (local $x i32) @@ -454,7 +459,7 @@ ;; CHECK-NEXT: (local.set $x ;; CHECK-NEXT: (i32.const 42) ;; CHECK-NEXT: ) - ;; CHECK-NEXT: (call $min + ;; CHECK-NEXT: (call $existing ;; CHECK-NEXT: (i32.const 42) ;; CHECK-NEXT: (i32.const 1) ;; CHECK-NEXT: (local.get $x) @@ -462,7 +467,7 @@ ;; CHECK-NEXT: (local.get $x) ;; CHECK-NEXT: ) ;; CHECK-NEXT: ) - ;; CHECK-NEXT: (call $min + ;; CHECK-NEXT: (call $log-branch ;; CHECK-NEXT: (i32.const 1) ;; CHECK-NEXT: (i32.const 1) ;; CHECK-NEXT: (local.get $1) @@ -484,7 +489,7 @@ (local.set $x (i32.const 42) ) - (call $min + (call $existing (i32.const 42) (i32.const 1) (local.get $x) diff --git a/test/lit/passes/optimize-instructions-branch-hints.wast b/test/lit/passes/optimize-instructions-branch-hints.wast deleted file mode 100644 index 270bf179216..00000000000 --- a/test/lit/passes/optimize-instructions-branch-hints.wast +++ /dev/null @@ -1,32 +0,0 @@ -;; NOTE: Assertions have been generated by update_lit_checks.py and should not be edited. -;; RUN: wasm-opt %s --optimize-instructions -all -S -o - | filecheck %s - -(module - ;; CHECK: (func $conditionals (type $0) (param $x i32) (result i32) - ;; CHECK-NEXT: (@metadata.code.branch_hint "\01") - ;; CHECK-NEXT: (if (result i32) - ;; CHECK-NEXT: (local.get $x) - ;; CHECK-NEXT: (then - ;; CHECK-NEXT: (i32.const 1337) - ;; CHECK-NEXT: ) - ;; CHECK-NEXT: (else - ;; CHECK-NEXT: (i32.const 42) - ;; CHECK-NEXT: ) - ;; CHECK-NEXT: ) - ;; CHECK-NEXT: ) - (func $conditionals (param $x i32) (result i32) - ;; When we flip the if, the hint should flip too. - (@metadata.code.branch_hint "\00") - (if (result i32) - (i32.eqz - (local.get $x) - ) - (then - (i32.const 42) - ) - (else - (i32.const 1337) - ) - ) - ) -) diff --git a/test/lit/passes/optimize-instructions_branch-hints-fold.wast b/test/lit/passes/optimize-instructions_branch-hints-fold.wast new file mode 100644 index 00000000000..6223817e59e --- /dev/null +++ b/test/lit/passes/optimize-instructions_branch-hints-fold.wast @@ -0,0 +1,268 @@ +;; NOTE: Assertions have been generated by update_lit_checks.py and should not be edited. + +;; RUN: wasm-opt %s --optimize-instructions -all -S -o - | filecheck %s + +;; Also verify that the "never-fold-or-reorder" flag is respected: when set, we +;; do not fold code together (important, as we keep one of the branch hints, and +;; it may be wrong, which can confuse the fuzzer), and we never reorder (which +;; can move a branch hint to execute before a trap, which can also cause the +;; fuzzer to alert). + +;; RUN: wasm-opt %s --optimize-instructions -all --pass-arg=optimize-instructions-never-fold-or-reorder -S -o - \ +;; RUN: | filecheck %s --check-prefix=NO_FO + +(module + ;; CHECK: (func $conditionals (type $1) (param $x i32) (result i32) + ;; CHECK-NEXT: (@metadata.code.branch_hint "\01") + ;; CHECK-NEXT: (if (result i32) + ;; CHECK-NEXT: (local.get $x) + ;; CHECK-NEXT: (then + ;; CHECK-NEXT: (i32.const 1337) + ;; CHECK-NEXT: ) + ;; CHECK-NEXT: (else + ;; CHECK-NEXT: (i32.const 42) + ;; CHECK-NEXT: ) + ;; CHECK-NEXT: ) + ;; CHECK-NEXT: ) + ;; NO_FO: (func $conditionals (type $1) (param $x i32) (result i32) + ;; NO_FO-NEXT: (@metadata.code.branch_hint "\01") + ;; NO_FO-NEXT: (if (result i32) + ;; NO_FO-NEXT: (local.get $x) + ;; NO_FO-NEXT: (then + ;; NO_FO-NEXT: (i32.const 1337) + ;; NO_FO-NEXT: ) + ;; NO_FO-NEXT: (else + ;; NO_FO-NEXT: (i32.const 42) + ;; NO_FO-NEXT: ) + ;; NO_FO-NEXT: ) + ;; NO_FO-NEXT: ) + (func $conditionals (param $x i32) (result i32) + ;; When we flip the if, the hint should flip too. + (@metadata.code.branch_hint "\00") + (if (result i32) + (i32.eqz + (local.get $x) + ) + (then + (i32.const 42) + ) + (else + (i32.const 1337) + ) + ) + ) + + ;; CHECK: (func $still-fold (type $0) (param $x i32) (param $y i32) + ;; CHECK-NEXT: (@metadata.code.branch_hint "\00") + ;; CHECK-NEXT: (if + ;; CHECK-NEXT: (local.get $y) + ;; CHECK-NEXT: (then + ;; CHECK-NEXT: (unreachable) + ;; CHECK-NEXT: ) + ;; CHECK-NEXT: ) + ;; CHECK-NEXT: ) + ;; NO_FO: (func $still-fold (type $0) (param $x i32) (param $y i32) + ;; NO_FO-NEXT: (if + ;; NO_FO-NEXT: (local.get $x) + ;; NO_FO-NEXT: (then + ;; NO_FO-NEXT: (@metadata.code.branch_hint "\00") + ;; NO_FO-NEXT: (if + ;; NO_FO-NEXT: (local.get $y) + ;; NO_FO-NEXT: (then + ;; NO_FO-NEXT: (unreachable) + ;; NO_FO-NEXT: ) + ;; NO_FO-NEXT: ) + ;; NO_FO-NEXT: ) + ;; NO_FO-NEXT: (else + ;; NO_FO-NEXT: (@metadata.code.branch_hint "\01") + ;; NO_FO-NEXT: (if + ;; NO_FO-NEXT: (local.get $y) + ;; NO_FO-NEXT: (then + ;; NO_FO-NEXT: (unreachable) + ;; NO_FO-NEXT: ) + ;; NO_FO-NEXT: ) + ;; NO_FO-NEXT: ) + ;; NO_FO-NEXT: ) + ;; NO_FO-NEXT: ) + (func $still-fold (param $x i32) (param $y i32) + ;; We fold if arms even if metadata differs (like LLVM). We do not fold if the + ;; flag was passed, however. + (if + (local.get $x) + (then + (@metadata.code.branch_hint "\00") + (if + (local.get $y) + (then + (unreachable) + ) + ) + ) + (else + (@metadata.code.branch_hint "\01") + (if + (local.get $y) + (then + (unreachable) + ) + ) + ) + ) + ) + + ;; CHECK: (func $yes-fold (type $0) (param $x i32) (param $y i32) + ;; CHECK-NEXT: (@metadata.code.branch_hint "\01") + ;; CHECK-NEXT: (if + ;; CHECK-NEXT: (local.get $y) + ;; CHECK-NEXT: (then + ;; CHECK-NEXT: (unreachable) + ;; CHECK-NEXT: ) + ;; CHECK-NEXT: ) + ;; CHECK-NEXT: ) + ;; NO_FO: (func $yes-fold (type $0) (param $x i32) (param $y i32) + ;; NO_FO-NEXT: (if + ;; NO_FO-NEXT: (local.get $x) + ;; NO_FO-NEXT: (then + ;; NO_FO-NEXT: (@metadata.code.branch_hint "\01") + ;; NO_FO-NEXT: (if + ;; NO_FO-NEXT: (local.get $y) + ;; NO_FO-NEXT: (then + ;; NO_FO-NEXT: (unreachable) + ;; NO_FO-NEXT: ) + ;; NO_FO-NEXT: ) + ;; NO_FO-NEXT: ) + ;; NO_FO-NEXT: (else + ;; NO_FO-NEXT: (@metadata.code.branch_hint "\01") + ;; NO_FO-NEXT: (if + ;; NO_FO-NEXT: (local.get $y) + ;; NO_FO-NEXT: (then + ;; NO_FO-NEXT: (unreachable) + ;; NO_FO-NEXT: ) + ;; NO_FO-NEXT: ) + ;; NO_FO-NEXT: ) + ;; NO_FO-NEXT: ) + ;; NO_FO-NEXT: ) + (func $yes-fold (param $x i32) (param $y i32) + ;; Now the hints match, so we definitely fold (without the flag). + (if + (local.get $x) + (then + (@metadata.code.branch_hint "\01") + (if + (local.get $y) + (then + (unreachable) + ) + ) + ) + (else + (@metadata.code.branch_hint "\01") + (if + (local.get $y) + (then + (unreachable) + ) + ) + ) + ) + ) + + ;; CHECK: (func $always-fold-select (type $2) (param $x i32) (param $y i32) (result i32) + ;; CHECK-NEXT: (@metadata.code.branch_hint "\00") + ;; CHECK-NEXT: (if (result i32) + ;; CHECK-NEXT: (local.get $x) + ;; CHECK-NEXT: (then + ;; CHECK-NEXT: (i32.const 10) + ;; CHECK-NEXT: ) + ;; CHECK-NEXT: (else + ;; CHECK-NEXT: (i32.const 20) + ;; CHECK-NEXT: ) + ;; CHECK-NEXT: ) + ;; CHECK-NEXT: ) + ;; NO_FO: (func $always-fold-select (type $2) (param $x i32) (param $y i32) (result i32) + ;; NO_FO-NEXT: (@metadata.code.branch_hint "\00") + ;; NO_FO-NEXT: (if (result i32) + ;; NO_FO-NEXT: (local.get $x) + ;; NO_FO-NEXT: (then + ;; NO_FO-NEXT: (i32.const 10) + ;; NO_FO-NEXT: ) + ;; NO_FO-NEXT: (else + ;; NO_FO-NEXT: (i32.const 20) + ;; NO_FO-NEXT: ) + ;; NO_FO-NEXT: ) + ;; NO_FO-NEXT: ) + (func $always-fold-select (param $x i32) (param $y i32) (result i32) + ;; A select with different metadata is still foldable: the code was executed + ;; anyhow, so it's fine if we execute just one of the two (we pick the first, + ;; arbitrarily). We do so even with the flag. + (select + (@metadata.code.branch_hint "\00") + (if (result i32) + (local.get $x) + (then + (i32.const 10) + ) + (else + (i32.const 20) + ) + ) + (@metadata.code.branch_hint "\01") + (if (result i32) + (local.get $x) + (then + (i32.const 10) + ) + (else + (i32.const 20) + ) + ) + (local.get $y) + ) + ) + + ;; CHECK: (func $ordering (type $3) (param $x i32) + ;; CHECK-NEXT: (drop + ;; CHECK-NEXT: (i32.add + ;; CHECK-NEXT: (local.get $x) + ;; CHECK-NEXT: (i32.const 42) + ;; CHECK-NEXT: ) + ;; CHECK-NEXT: ) + ;; CHECK-NEXT: (drop + ;; CHECK-NEXT: (i32.add + ;; CHECK-NEXT: (local.get $x) + ;; CHECK-NEXT: (i32.const 42) + ;; CHECK-NEXT: ) + ;; CHECK-NEXT: ) + ;; CHECK-NEXT: ) + ;; NO_FO: (func $ordering (type $3) (param $x i32) + ;; NO_FO-NEXT: (drop + ;; NO_FO-NEXT: (i32.add + ;; NO_FO-NEXT: (local.get $x) + ;; NO_FO-NEXT: (i32.const 42) + ;; NO_FO-NEXT: ) + ;; NO_FO-NEXT: ) + ;; NO_FO-NEXT: (drop + ;; NO_FO-NEXT: (i32.add + ;; NO_FO-NEXT: (i32.const 42) + ;; NO_FO-NEXT: (local.get $x) + ;; NO_FO-NEXT: ) + ;; NO_FO-NEXT: ) + ;; NO_FO-NEXT: ) + (func $ordering (param $x i32) + ;; Normally we canonicalize the sides of a binary like this (so after the + ;; pass, both the below expressions would be identical), but we refrain from + ;; doing so with the flag. + (drop + (i32.add + (local.get $x) + (i32.const 42) + ) + ) + (drop + (i32.add + (i32.const 42) + (local.get $x) + ) + ) + ) +) diff --git a/test/lit/passes/remove-unused-brs_branch-hints-unconditionalize.wast b/test/lit/passes/remove-unused-brs_branch-hints-unconditionalize.wast new file mode 100644 index 00000000000..145a18deccb --- /dev/null +++ b/test/lit/passes/remove-unused-brs_branch-hints-unconditionalize.wast @@ -0,0 +1,213 @@ +;; NOTE: Assertions have been generated by update_lit_checks.py and should not be edited. + +;; RUN: wasm-opt %s --remove-unused-brs -all -S -o - \ +;; RUN: | filecheck %s +;; RUN: wasm-opt %s --remove-unused-brs --pass-arg=remove-unused-brs-never-unconditionalize -all -S -o - \ +;; RUN: | filecheck %s --check-prefix=NO_UN + +;; Verify that the "never-unconditionalize" flag is respected: when set, we do +;; not run code unconditionally that previously might not have run. This is +;; important as the branch hint in un-executed code may be right or wrong, which +;; can confuse the fuzzer. + +(module + ;; CHECK: (func $selectify (type $0) (param $x i32) (param $y i32) (result i32) + ;; CHECK-NEXT: (select + ;; CHECK-NEXT: (local.get $y) + ;; CHECK-NEXT: (@metadata.code.branch_hint "\01") + ;; CHECK-NEXT: (if (result i32) + ;; CHECK-NEXT: (local.get $y) + ;; CHECK-NEXT: (then + ;; CHECK-NEXT: (i32.const 10) + ;; CHECK-NEXT: ) + ;; CHECK-NEXT: (else + ;; CHECK-NEXT: (block $out (result i32) + ;; CHECK-NEXT: (nop) + ;; CHECK-NEXT: (i32.const 20) + ;; CHECK-NEXT: ) + ;; CHECK-NEXT: ) + ;; CHECK-NEXT: ) + ;; CHECK-NEXT: (local.get $x) + ;; CHECK-NEXT: ) + ;; CHECK-NEXT: ) + ;; NO_UN: (func $selectify (type $0) (param $x i32) (param $y i32) (result i32) + ;; NO_UN-NEXT: (if (result i32) + ;; NO_UN-NEXT: (local.get $x) + ;; NO_UN-NEXT: (then + ;; NO_UN-NEXT: (local.get $y) + ;; NO_UN-NEXT: ) + ;; NO_UN-NEXT: (else + ;; NO_UN-NEXT: (@metadata.code.branch_hint "\01") + ;; NO_UN-NEXT: (if (result i32) + ;; NO_UN-NEXT: (local.get $y) + ;; NO_UN-NEXT: (then + ;; NO_UN-NEXT: (i32.const 10) + ;; NO_UN-NEXT: ) + ;; NO_UN-NEXT: (else + ;; NO_UN-NEXT: (block $out (result i32) + ;; NO_UN-NEXT: (nop) + ;; NO_UN-NEXT: (i32.const 20) + ;; NO_UN-NEXT: ) + ;; NO_UN-NEXT: ) + ;; NO_UN-NEXT: ) + ;; NO_UN-NEXT: ) + ;; NO_UN-NEXT: ) + ;; NO_UN-NEXT: ) + (func $selectify (param $x i32) (param $y i32) (result i32) + ;; This if can be a select, but the nested if's branch hint will then + ;; always execute, which we should avoid when the flag is passed. + (if (result i32) + (local.get $x) + (then + (local.get $y) + ) + (else + (block $out (result i32) + (@metadata.code.branch_hint "\01") + (if + (local.get $y) + (then + (br $out + (i32.const 10) + ) + ) + ) + (i32.const 20) + ) + ) + ) + ) + + ;; CHECK: (func $if-select (type $1) (param $x i32) (param $y i32) + ;; CHECK-NEXT: (block $out + ;; CHECK-NEXT: (br_if $out + ;; CHECK-NEXT: (select + ;; CHECK-NEXT: (local.get $x) + ;; CHECK-NEXT: (i32.const 0) + ;; CHECK-NEXT: (local.get $y) + ;; CHECK-NEXT: ) + ;; CHECK-NEXT: ) + ;; CHECK-NEXT: ) + ;; CHECK-NEXT: ) + ;; NO_UN: (func $if-select (type $1) (param $x i32) (param $y i32) + ;; NO_UN-NEXT: (if + ;; NO_UN-NEXT: (local.get $x) + ;; NO_UN-NEXT: (then + ;; NO_UN-NEXT: (block $out + ;; NO_UN-NEXT: (br_if $out + ;; NO_UN-NEXT: (local.get $y) + ;; NO_UN-NEXT: ) + ;; NO_UN-NEXT: ) + ;; NO_UN-NEXT: ) + ;; NO_UN-NEXT: ) + ;; NO_UN-NEXT: ) + (func $if-select (param $x i32) (param $y i32) + ;; The br_if can be combined with the if using a select, but not when the + ;; flag is passed. + (block $out + (if + (local.get $x) + (then + (br_if $out + (local.get $y) + ) + ) + ) + ) + ) + + ;; CHECK: (func $if-select-2 (type $1) (param $x i32) (param $y i32) + ;; CHECK-NEXT: (if + ;; CHECK-NEXT: (select + ;; CHECK-NEXT: (local.get $x) + ;; CHECK-NEXT: (i32.const 0) + ;; CHECK-NEXT: (local.get $y) + ;; CHECK-NEXT: ) + ;; CHECK-NEXT: (then + ;; CHECK-NEXT: (nop) + ;; CHECK-NEXT: ) + ;; CHECK-NEXT: ) + ;; CHECK-NEXT: ) + ;; NO_UN: (func $if-select-2 (type $1) (param $x i32) (param $y i32) + ;; NO_UN-NEXT: (if + ;; NO_UN-NEXT: (local.get $x) + ;; NO_UN-NEXT: (then + ;; NO_UN-NEXT: (if + ;; NO_UN-NEXT: (local.get $y) + ;; NO_UN-NEXT: (then + ;; NO_UN-NEXT: (nop) + ;; NO_UN-NEXT: ) + ;; NO_UN-NEXT: ) + ;; NO_UN-NEXT: ) + ;; NO_UN-NEXT: ) + ;; NO_UN-NEXT: ) + (func $if-select-2 (param $x i32) (param $y i32) + ;; The if conditions can be combined into one if with a select, but not when + ;; the flag is passed. + (if + (local.get $x) + (then + (if + (local.get $y) + (then + (nop) + ) + ) + ) + ) + ) + + ;; CHECK: (func $nothing (type $2) + ;; CHECK-NEXT: (nop) + ;; CHECK-NEXT: ) + ;; NO_UN: (func $nothing (type $2) + ;; NO_UN-NEXT: (nop) + ;; NO_UN-NEXT: ) + (func $nothing + ;; Helper for below. + (nop) + ) + + ;; CHECK: (func $restructure-br_if-value-effectful (type $0) (param $x i32) (param $y i32) (result i32) + ;; CHECK-NEXT: (select + ;; CHECK-NEXT: (block (result i32) + ;; CHECK-NEXT: (call $nothing) + ;; CHECK-NEXT: (local.get $y) + ;; CHECK-NEXT: ) + ;; CHECK-NEXT: (block $x (result i32) + ;; CHECK-NEXT: (nop) + ;; CHECK-NEXT: (i32.const 0) + ;; CHECK-NEXT: ) + ;; CHECK-NEXT: (local.get $x) + ;; CHECK-NEXT: ) + ;; CHECK-NEXT: ) + ;; NO_UN: (func $restructure-br_if-value-effectful (type $0) (param $x i32) (param $y i32) (result i32) + ;; NO_UN-NEXT: (block $x (result i32) + ;; NO_UN-NEXT: (drop + ;; NO_UN-NEXT: (br_if $x + ;; NO_UN-NEXT: (block (result i32) + ;; NO_UN-NEXT: (call $nothing) + ;; NO_UN-NEXT: (local.get $y) + ;; NO_UN-NEXT: ) + ;; NO_UN-NEXT: (local.get $x) + ;; NO_UN-NEXT: ) + ;; NO_UN-NEXT: ) + ;; NO_UN-NEXT: (i32.const 0) + ;; NO_UN-NEXT: ) + ;; NO_UN-NEXT: ) + (func $restructure-br_if-value-effectful (param $x i32) (param $y i32) (result i32) + ;; We can restructure this to a select, but not when the flag is passed. + (block $x (result i32) + (drop + (br_if $x + (block (result i32) + (call $nothing) + (local.get $y) + ) + (local.get $x) + ) + ) + (i32.const 0) + ) + ) +)