From ef1d4d4b06d5a95efc92cfece16573e089cd6fa6 Mon Sep 17 00:00:00 2001 From: Alon Zakai Date: Fri, 19 Nov 2021 15:26:31 -0800 Subject: [PATCH 1/6] work --- src/passes/SimplifyGlobals.cpp | 127 ++++++++++++------ .../simplify-globals-read_only_to_write.wast | 121 +++++++++++++++-- 2 files changed, 202 insertions(+), 46 deletions(-) diff --git a/src/passes/SimplifyGlobals.cpp b/src/passes/SimplifyGlobals.cpp index be7cc6ca1d0..57a0b9b69d1 100644 --- a/src/passes/SimplifyGlobals.cpp +++ b/src/passes/SimplifyGlobals.cpp @@ -61,13 +61,15 @@ struct GlobalInfo { std::atomic read{0}; // How many times the global is "read, but only to write", that is, is used in - // this pattern: + // something like this pattern: // // if (global == X) { global = Y } // - // where X and Y have no side effects. If all we have are such reads only to - // write then the global is really not necessary, even though there are both - // reads and writes of it. + // We don't allow any side effects aside from writing to |global| in the if + // body. But we do allow other things to happen in the if condition, so long + // as the global is read only in order to decide to write that same global. + // If all we have are such reads only to write then the global is really not + // necessary, even though there are both reads and writes of it. // // This pattern can show up in global initialization code, where in the block // alongside "global = Y" there was some useful code, but the optimizer @@ -108,55 +110,104 @@ struct GlobalUseScanner : public WalkerPass> { } auto global = - firstOnlyReadsGlobalWhichSecondOnlyWrites(curr->condition, curr->ifTrue); + conditionReadsGlobalWhichIsOnlyWritten(curr->condition, curr->ifTrue); if (global.is()) { // This is exactly the pattern we sought! (*infos)[global].readOnlyToWrite++; } } - // Checks if the first expression only reads a certain global, and has no - // other effects, and the second only writes that same global, and also has no - // other effects. Returns the global name if so, or a null name otherwise. - Name firstOnlyReadsGlobalWhichSecondOnlyWrites(Expression* first, - Expression* second) { - // See if reading a specific global is the only effect the first has. - EffectAnalyzer firstEffects(getPassOptions(), *getModule(), first); - - if (firstEffects.immutableGlobalsRead.size() + - firstEffects.mutableGlobalsRead.size() != - 1) { + // Checks if second only writes some global, and the first reads that global + // in order to decide if to write it. It doesn't matter how the first uses + // that read value from that global, so long as it is only used to determine + // the result of the first expression. This is a simple case: + // + // | first | | second | + // if (global == 0) { global = 1; } + // + // But we also allow this: + // + // if (global % 17 < 4) { global = 1 } + // + // What we want to disallow is using the global to actually do something that + // is noticeeable *aside* from writing the global, like this: + // + // if (global ? foo() : bar()) { .. } + // + // Here ? : is another nested if, and we end up running different code based + // on global, which is noticeable. + // + // Returns the global name if things like up, or a null name otherwise. + Name conditionReadsGlobalWhichIsOnlyWritten(Expression* condition, + Expression* code) { + // See if writing a global is the only effect the code has. (Note that we + // don't need to care about the case where the code has no effects at + // all - other passes would handle that trivial situation.) + EffectAnalyzer codeEffects(getPassOptions(), *getModule(), code); + if (codeEffects.globalsWritten.size() != 1) { return Name(); } - Name global; - if (firstEffects.immutableGlobalsRead.size() == 1) { - global = *firstEffects.immutableGlobalsRead.begin(); - firstEffects.immutableGlobalsRead.clear(); - } else { - global = *firstEffects.mutableGlobalsRead.begin(); - firstEffects.mutableGlobalsRead.clear(); - } - if (firstEffects.hasAnything()) { + auto writtenGlobal = *codeEffects.globalsWritten.begin(); + codeEffects.globalsWritten.clear(); + if (codeEffects.hasAnything()) { return Name(); } - // See if writing the same global is the only effect the second has. (Note - // that we don't need to care about the case where the second has no effects - // at all - other passes would handle that trivial situation.) - EffectAnalyzer secondEffects(getPassOptions(), *getModule(), second); - if (secondEffects.globalsWritten.size() != 1) { + // See if we read that global in the condition expression. + EffectAnalyzer conditionEffects(getPassOptions(), *getModule(), condition); + if (!conditionEffects.mutableGlobalsRead.count(writtenGlobal)) { return Name(); } - auto writtenGlobal = *secondEffects.globalsWritten.begin(); - if (writtenGlobal != global) { - return Name(); + + // See if the condition has any other effects that could be a problem. If it has + // no side effects at all, then there is nothing noticeable. (Or, if it has + // side effects the optimizer is allowed to remove, that is fine too - we + // can ignore them too.) + if (!conditionEffects.hasUnremovableSideEffects()) { + // No side effects means there is nothing noticeable. + return writtenGlobal; } - secondEffects.globalsWritten.clear(); - if (secondEffects.hasAnything()) { - return Name(); + + // There are unremovable side effects of some form. Handle at least one more + // case that is fairly common in practice, where multiple ifs are fused + // together to form this: + // + // if (global ? .. : ..) { global = 1 } + // + // Here ? is a select inside the if, which combines some check on |global| + // with some other check. For example, + // + // global ? 0 : x = 10 + // + // That has the side effect of writing to x, but that is ok, all children of + // a select execute unconditionally anyhow, so |global|'s value is not used + // to cause anything noticeable. To check that, see if the get of the global + // is a child of condition, and that condition does not do anything dangerous with + // that value. + // + // Also, first look through a unary such as an EqZ. + if (auto* unary = condition->dynCast()) { + condition = unary->value; + } + for (auto* conditionChild : ChildIterator(condition)) { + if (auto* get = conditionChild->dynCast()) { + if (get->name == writtenGlobal) { + // The get is indeed a child of condition. Verify that condition uses that + // value in a safe way. + EffectAnalyzer immediateConditionEffects(getPassOptions(), *getModule()); + immediateConditionEffects.visit(condition); + if (!immediateConditionEffects.hasUnremovableSideEffects()) { + return writtenGlobal; + } + + // Otherwise, exit: there is no point scanning the other children, as + // we found the one we were looking for. + break; + } + } } - return global; + return Name(); } void visitFunction(Function* curr) { @@ -190,7 +241,7 @@ struct GlobalUseScanner : public WalkerPass> { } auto global = - firstOnlyReadsGlobalWhichSecondOnlyWrites(iff->condition, list[1]); + conditionReadsGlobalWhichIsOnlyWritten(iff->condition, list[1]); if (global.is()) { // This is exactly the pattern we sought! (*infos)[global].readOnlyToWrite++; diff --git a/test/lit/passes/simplify-globals-read_only_to_write.wast b/test/lit/passes/simplify-globals-read_only_to_write.wast index 662c2fd0583..c92153367ff 100644 --- a/test/lit/passes/simplify-globals-read_only_to_write.wast +++ b/test/lit/passes/simplify-globals-read_only_to_write.wast @@ -114,22 +114,22 @@ (module ;; CHECK: (type $none_=>_none (func)) - ;; CHECK: (global $global (mut i32) (i32.const 0)) + ;; CHECK: (global $global i32 (i32.const 0)) (global $global (mut i32) (i32.const 0)) - ;; CHECK: (global $other (mut i32) (i32.const 0)) + ;; CHECK: (global $other i32 (i32.const 0)) (global $other (mut i32) (i32.const 0)) ;; CHECK: (func $side-effects-in-condition ;; CHECK-NEXT: (if ;; CHECK-NEXT: (block $block (result i32) - ;; CHECK-NEXT: (global.set $other + ;; CHECK-NEXT: (drop ;; CHECK-NEXT: (i32.const 2) ;; CHECK-NEXT: ) ;; CHECK-NEXT: (drop ;; CHECK-NEXT: (i32.const 2) ;; CHECK-NEXT: ) - ;; CHECK-NEXT: (global.get $global) + ;; CHECK-NEXT: (i32.const 0) ;; CHECK-NEXT: ) - ;; CHECK-NEXT: (global.set $global + ;; CHECK-NEXT: (drop ;; CHECK-NEXT: (i32.const 1) ;; CHECK-NEXT: ) ;; CHECK-NEXT: ) @@ -358,18 +358,18 @@ (module ;; CHECK: (type $none_=>_none (func)) - ;; CHECK: (global $once (mut i32) (i32.const 0)) + ;; CHECK: (global $once i32 (i32.const 0)) (global $once (mut i32) (i32.const 0)) ;; CHECK: (func $clinit ;; CHECK-NEXT: (if ;; CHECK-NEXT: (block $block (result i32) ;; CHECK-NEXT: (unreachable) - ;; CHECK-NEXT: (global.get $once) + ;; CHECK-NEXT: (i32.const 0) ;; CHECK-NEXT: ) ;; CHECK-NEXT: (return) ;; CHECK-NEXT: ) - ;; CHECK-NEXT: (global.set $once + ;; CHECK-NEXT: (drop ;; CHECK-NEXT: (i32.const 1) ;; CHECK-NEXT: ) ;; CHECK-NEXT: ) @@ -387,3 +387,108 @@ ) ) ) + +(module + (memory 1 1) + + ;; CHECK: (type $none_=>_none (func)) + + ;; CHECK: (global $once i32 (i32.const 0)) + (global $once (mut i32) (i32.const 0)) + + ;; CHECK: (memory $0 1 1) + + ;; CHECK: (func $test + ;; CHECK-NEXT: (local $x i32) + ;; CHECK-NEXT: (local $y i32) + ;; CHECK-NEXT: (if + ;; CHECK-NEXT: (select + ;; CHECK-NEXT: (local.tee $x + ;; CHECK-NEXT: (i32.const 1) + ;; CHECK-NEXT: ) + ;; CHECK-NEXT: (i32.load + ;; CHECK-NEXT: (i32.const 2) + ;; CHECK-NEXT: ) + ;; CHECK-NEXT: (i32.const 0) + ;; CHECK-NEXT: ) + ;; CHECK-NEXT: (drop + ;; CHECK-NEXT: (i32.const 1) + ;; CHECK-NEXT: ) + ;; CHECK-NEXT: ) + ;; CHECK-NEXT: ) + (func $test + (local $x i32) + (local $y i32) + (if + ;; The condition has various side effects, but they do not cause any + ;; problems: $once is read in a way that only affects whether we get to + ;; the set of $once - its value does not cause anything else observable. + (select + (local.tee $x + (i32.const 1) + ) + (i32.load + (i32.const 2) + ) + (global.get $once) + ) + (global.set $once + (i32.const 1) + ) + ) + ) +) + +(module + (memory 1 1) + + ;; CHECK: (type $none_=>_none (func)) + + ;; CHECK: (global $once i32 (i32.const 0)) + (global $once (mut i32) (i32.const 0)) + + ;; CHECK: (memory $0 1 1) + + ;; CHECK: (func $test + ;; CHECK-NEXT: (local $x i32) + ;; CHECK-NEXT: (local $y i32) + ;; CHECK-NEXT: (if + ;; CHECK-NEXT: (i32.eqz + ;; CHECK-NEXT: (select + ;; CHECK-NEXT: (local.tee $x + ;; CHECK-NEXT: (i32.const 1) + ;; CHECK-NEXT: ) + ;; CHECK-NEXT: (i32.load + ;; CHECK-NEXT: (i32.const 2) + ;; CHECK-NEXT: ) + ;; CHECK-NEXT: (i32.const 0) + ;; CHECK-NEXT: ) + ;; CHECK-NEXT: ) + ;; CHECK-NEXT: (drop + ;; CHECK-NEXT: (i32.const 1) + ;; CHECK-NEXT: ) + ;; CHECK-NEXT: ) + ;; CHECK-NEXT: ) + (func $test + (local $x i32) + (local $y i32) + (if + ;; As above, but with an extra eqz + (i32.eqz + (select + (local.tee $x + (i32.const 1) + ) + (i32.load + (i32.const 2) + ) + (global.get $once) + ) + ) + (global.set $once + (i32.const 1) + ) + ) + ) +) + From fc03f6a4e6467ee38c64b8cb526e30c4f4190a3a Mon Sep 17 00:00:00 2001 From: Alon Zakai Date: Tue, 30 Nov 2021 15:40:38 -0800 Subject: [PATCH 2/6] work --- src/passes/SimplifyGlobals.cpp | 126 ++++++++++++++++++--------------- 1 file changed, 68 insertions(+), 58 deletions(-) diff --git a/src/passes/SimplifyGlobals.cpp b/src/passes/SimplifyGlobals.cpp index bd7020bda09..9eee4ce8096 100644 --- a/src/passes/SimplifyGlobals.cpp +++ b/src/passes/SimplifyGlobals.cpp @@ -69,11 +69,11 @@ struct GlobalInfo { // // if (global == X) { global = Y } // - // We don't allow any side effects aside from writing to |global| in the if - // body. But we do allow other things to happen in the if condition, so long - // as the global is read only in order to decide to write that same global. - // If all we have are such reads only to write then the global is really not - // necessary, even though there are both reads and writes of it. + // The if's condition only uses |global| in order to decide to write to that + // same global, so it is "read, but only to write." If all we have are such + // reads only to write then the global is really not necessary, even though + // there are both reads and writes of it, and regardless of what the written + // values are etc. // // This pattern can show up in global initialization code, where in the block // alongside "global = Y" there was some useful code, but the optimizer @@ -127,22 +127,21 @@ struct GlobalUseScanner : public WalkerPass> { } auto global = - conditionReadsGlobalWhichIsOnlyWritten(curr->condition, curr->ifTrue); + readsGlobalOnlyToWriteIt(curr->condition, curr->ifTrue); if (global.is()) { // This is exactly the pattern we sought! (*infos)[global].readOnlyToWrite++; } } - // Checks if second only writes some global, and the first reads that global - // in order to decide if to write it. It doesn't matter how the first uses - // that read value from that global, so long as it is only used to determine - // the result of the first expression. This is a simple case: + // Given a condition and some code that is executed based on the condition, + // check if the condition reads from some global in order to make the decision + // whether to run that code, and that code only writes to that global, which + // means the global is "read, but only to be written." // - // | first | | second | - // if (global == 0) { global = 1; } - // - // But we also allow this: + // The condition may also do other things than read from that global - it may + // compare it to a value, or negate it, or anything else, so long as the value + // of the global is only used to decide to run the code, like this: // // if (global % 17 < 4) { global = 1 } // @@ -152,11 +151,12 @@ struct GlobalUseScanner : public WalkerPass> { // if (global ? foo() : bar()) { .. } // // Here ? : is another nested if, and we end up running different code based - // on global, which is noticeable. + // on global, which is noticeable: the global is *not* only read in order to + // write that global, but also for other reasons. // // Returns the global name if things like up, or a null name otherwise. - Name conditionReadsGlobalWhichIsOnlyWritten(Expression* condition, - Expression* code) { + Name readsGlobalOnlyToWriteIt(Expression* condition, + Expression* code) { // See if writing a global is the only effect the code has. (Note that we // don't need to care about the case where the code has no effects at // all - other passes would handle that trivial situation.) @@ -176,55 +176,65 @@ struct GlobalUseScanner : public WalkerPass> { return Name(); } - // See if the condition has any other effects that could be a problem. If it has - // no side effects at all, then there is nothing noticeable. (Or, if it has - // side effects the optimizer is allowed to remove, that is fine too - we - // can ignore them too.) + // If the condition has no other (non-removable) effects other than reading + // that global then we have found what we looked for. if (!conditionEffects.hasUnremovableSideEffects()) { - // No side effects means there is nothing noticeable. return writtenGlobal; } - // There are unremovable side effects of some form. Handle at least one more - // case that is fairly common in practice, where multiple ifs are fused - // together to form this: - // - // if (global ? .. : ..) { global = 1 } - // - // Here ? is a select inside the if, which combines some check on |global| - // with some other check. For example, - // - // global ? 0 : x = 10 + // There are unremovable side effects of some form. However, they may not + // be related to the reading of the global, that is, the global's value may + // not flow to anything that uses it in a dangerous way. It *would* be + // dangerous for the global's value to flow into a nested if condition, as + // mentioned in the comment earlier, but if it flows into an if arm for + // example then that is safe, so long as the final place it flows out to is + // the condition. // - // That has the side effect of writing to x, but that is ok, all children of - // a select execute unconditionally anyhow, so |global|'s value is not used - // to cause anything noticeable. To check that, see if the get of the global - // is a child of condition, and that condition does not do anything dangerous with - // that value. - // - // Also, first look through a unary such as an EqZ. - if (auto* unary = condition->dynCast()) { - condition = unary->value; - } - for (auto* conditionChild : ChildIterator(condition)) { - if (auto* get = conditionChild->dynCast()) { - if (get->name == writtenGlobal) { - // The get is indeed a child of condition. Verify that condition uses that - // value in a safe way. - EffectAnalyzer immediateConditionEffects(getPassOptions(), *getModule()); - immediateConditionEffects.visit(condition); - if (!immediateConditionEffects.hasUnremovableSideEffects()) { - return writtenGlobal; + // To check this, find the get of the global in the condition, and look up + // through its parents to see how the global's value is used. + struct FlowScanner : public ExpressionStackWalker> { + Name writtenGlobal; + PassOptions& passOptions; + Module& wasm; + + bool ok = true; + + void visitExpression(Expression* curr) { + if (auto* get = curr->dynCast()) { + if (get->name == writtenGlobal) { + // We found the get of the global. Check where its value flows to, + // and how it is used there. + assert(expressionStack.back() == get); + for (Index i = 0; i < expressionStack.size() - 1; i++) { + // Consider one pair of parent->child, and check if the parent + // causes any problems when the child's value reaches it. + auto* parent = expressionStack[i]; + auto* child = expressionStack[i + 1]; + EffectAnalyzer parentEffects(passOptions, wasm); + parentEffects.visit(parent); + if (parentEffects.hasUnremovableSideEffects()) { + // The parent has some side effect, and the child's value may + // be used to determine its manner, so this is dangerous. + ok = false; + break; + } + + if (auto* iff = parent->dynCast()) { + if (iff->condition == child) { + // The child is used to decide what code to run, which is + // dangerous. + ok = false; + break; + } + } } - - // Otherwise, exit: there is no point scanning the other children, as - // we found the one we were looking for. - break; } } - } + }; - return Name(); + FlowScanner scanner{writtenGlobal, getPassOptions(), *getModule()}; + scanner.walk(condition); + return scanner.ok ? writtenGlobal : Name(); } void visitFunction(Function* curr) { @@ -258,7 +268,7 @@ struct GlobalUseScanner : public WalkerPass> { } auto global = - conditionReadsGlobalWhichIsOnlyWritten(iff->condition, list[1]); + readsGlobalOnlyToWriteIt(iff->condition, list[1]); if (global.is()) { // This is exactly the pattern we sought! (*infos)[global].readOnlyToWrite++; From b614069a450bc5df79072c2fa5d41971847feae8 Mon Sep 17 00:00:00 2001 From: Alon Zakai Date: Tue, 30 Nov 2021 15:42:42 -0800 Subject: [PATCH 3/6] builds --- src/passes/SimplifyGlobals.cpp | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/src/passes/SimplifyGlobals.cpp b/src/passes/SimplifyGlobals.cpp index 9eee4ce8096..933182ac497 100644 --- a/src/passes/SimplifyGlobals.cpp +++ b/src/passes/SimplifyGlobals.cpp @@ -197,6 +197,8 @@ struct GlobalUseScanner : public WalkerPass> { PassOptions& passOptions; Module& wasm; + FlowScanner(Name writtenGlobal, PassOptions& passOptions, Module& wasm) : writtenGlobal(writtenGlobal), passOptions(passOptions), wasm(wasm) {} + bool ok = true; void visitExpression(Expression* curr) { @@ -221,10 +223,11 @@ struct GlobalUseScanner : public WalkerPass> { if (auto* iff = parent->dynCast()) { if (iff->condition == child) { - // The child is used to decide what code to run, which is - // dangerous. - ok = false; - break; + // The child is used to decide what code to run, which is + // dangerous. + ok = false; + break; + } } } } @@ -232,7 +235,7 @@ struct GlobalUseScanner : public WalkerPass> { } }; - FlowScanner scanner{writtenGlobal, getPassOptions(), *getModule()}; + FlowScanner scanner(writtenGlobal, getPassOptions(), *getModule()); scanner.walk(condition); return scanner.ok ? writtenGlobal : Name(); } From 9b8c9f5d5d645520a89715a07ec1b2c593508861 Mon Sep 17 00:00:00 2001 From: Alon Zakai Date: Tue, 30 Nov 2021 15:52:32 -0800 Subject: [PATCH 4/6] work --- .../simplify-globals-read_only_to_write.wast | 180 +++++++++++------- 1 file changed, 108 insertions(+), 72 deletions(-) diff --git a/test/lit/passes/simplify-globals-read_only_to_write.wast b/test/lit/passes/simplify-globals-read_only_to_write.wast index c92153367ff..a57b84c3180 100644 --- a/test/lit/passes/simplify-globals-read_only_to_write.wast +++ b/test/lit/passes/simplify-globals-read_only_to_write.wast @@ -110,41 +110,6 @@ ) ) ) -;; Side effects in the condition prevent the read-only-to-write optimization. -(module - ;; CHECK: (type $none_=>_none (func)) - - ;; CHECK: (global $global i32 (i32.const 0)) - (global $global (mut i32) (i32.const 0)) - ;; CHECK: (global $other i32 (i32.const 0)) - (global $other (mut i32) (i32.const 0)) - ;; CHECK: (func $side-effects-in-condition - ;; CHECK-NEXT: (if - ;; CHECK-NEXT: (block $block (result i32) - ;; CHECK-NEXT: (drop - ;; CHECK-NEXT: (i32.const 2) - ;; CHECK-NEXT: ) - ;; CHECK-NEXT: (drop - ;; CHECK-NEXT: (i32.const 2) - ;; CHECK-NEXT: ) - ;; CHECK-NEXT: (i32.const 0) - ;; CHECK-NEXT: ) - ;; CHECK-NEXT: (drop - ;; CHECK-NEXT: (i32.const 1) - ;; CHECK-NEXT: ) - ;; CHECK-NEXT: ) - ;; CHECK-NEXT: ) - (func $side-effects-in-condition - (if - (block (result i32) - (global.set $other (i32.const 2)) - (drop (global.get $other)) - (global.get $global) - ) - (global.set $global (i32.const 1)) - ) - ) -) ;; Side effects in the body prevent the read-only-to-write optimization. (module ;; CHECK: (type $none_=>_none (func)) @@ -388,27 +353,64 @@ ) ) +;; Using the global's value in a way that can cause side effects prevents the +;; read-only-to-write optimization. (module - (memory 1 1) - ;; CHECK: (type $none_=>_none (func)) - ;; CHECK: (global $once i32 (i32.const 0)) - (global $once (mut i32) (i32.const 0)) + ;; CHECK: (type $none_=>_i32 (func (result i32))) - ;; CHECK: (memory $0 1 1) + ;; CHECK: (global $global (mut i32) (i32.const 0)) + (global $global (mut i32) (i32.const 0)) + ;; CHECK: (global $other i32 (i32.const 0)) + (global $other (mut i32) (i32.const 0)) + ;; CHECK: (func $side-effects-in-condition + ;; CHECK-NEXT: (if + ;; CHECK-NEXT: (if (result i32) + ;; CHECK-NEXT: (global.get $global) + ;; CHECK-NEXT: (call $foo) + ;; CHECK-NEXT: (i32.const 1) + ;; CHECK-NEXT: ) + ;; CHECK-NEXT: (global.set $global + ;; CHECK-NEXT: (i32.const 1) + ;; CHECK-NEXT: ) + ;; CHECK-NEXT: ) + ;; CHECK-NEXT: ) + (func $side-effects-in-condition + (if + (if (result i32) + (global.get $global) ;; the global's value may cause foo() to be called + (call $foo) + (i32.const 1) + ) + (global.set $global (i32.const 1)) + ) + ) - ;; CHECK: (func $test - ;; CHECK-NEXT: (local $x i32) - ;; CHECK-NEXT: (local $y i32) + ;; CHECK: (func $foo (result i32) + ;; CHECK-NEXT: (unreachable) + ;; CHECK-NEXT: ) + (func $foo (result i32) + (unreachable) + ) +) + +;; As above, but now the global's value is not the condition of the if, so there +;; is no problem. +(module + ;; CHECK: (type $none_=>_none (func)) + + ;; CHECK: (type $none_=>_i32 (func (result i32))) + + ;; CHECK: (global $global i32 (i32.const 0)) + (global $global (mut i32) (i32.const 0)) + ;; CHECK: (global $other i32 (i32.const 0)) + (global $other (mut i32) (i32.const 0)) + ;; CHECK: (func $side-effects-in-condition-2 ;; CHECK-NEXT: (if - ;; CHECK-NEXT: (select - ;; CHECK-NEXT: (local.tee $x - ;; CHECK-NEXT: (i32.const 1) - ;; CHECK-NEXT: ) - ;; CHECK-NEXT: (i32.load - ;; CHECK-NEXT: (i32.const 2) - ;; CHECK-NEXT: ) + ;; CHECK-NEXT: (if (result i32) + ;; CHECK-NEXT: (call $foo) + ;; CHECK-NEXT: (i32.const 1) ;; CHECK-NEXT: (i32.const 0) ;; CHECK-NEXT: ) ;; CHECK-NEXT: (drop @@ -416,29 +418,59 @@ ;; CHECK-NEXT: ) ;; CHECK-NEXT: ) ;; CHECK-NEXT: ) - (func $test - (local $x i32) - (local $y i32) + (func $side-effects-in-condition-2 (if - ;; The condition has various side effects, but they do not cause any - ;; problems: $once is read in a way that only affects whether we get to - ;; the set of $once - its value does not cause anything else observable. - (select - (local.tee $x - (i32.const 1) - ) - (i32.load - (i32.const 2) - ) - (global.get $once) - ) - (global.set $once + (if (result i32) + (call $foo) ;; these side effects are not a problem, as the global's + ;; value cannot reach them. (i32.const 1) + (global.get $global) ;; the global's value flows out through the if, + ;; safely ) + (global.set $global (i32.const 1)) + ) + ) + + ;; CHECK: (func $foo (result i32) + ;; CHECK-NEXT: (unreachable) + ;; CHECK-NEXT: ) + (func $foo (result i32) + (unreachable) + ) +) + +;; As above, but now the global's value flows into a side effect. +(module + ;; CHECK: (type $none_=>_none (func)) + + ;; CHECK: (global $global (mut i32) (i32.const 0)) + (global $global (mut i32) (i32.const 0)) + ;; CHECK: (global $other i32 (i32.const 0)) + (global $other (mut i32) (i32.const 0)) + ;; CHECK: (func $side-effects-in-condition-3 + ;; CHECK-NEXT: (local $temp i32) + ;; CHECK-NEXT: (if + ;; CHECK-NEXT: (local.tee $temp + ;; CHECK-NEXT: (global.get $global) + ;; CHECK-NEXT: ) + ;; CHECK-NEXT: (global.set $global + ;; CHECK-NEXT: (i32.const 1) + ;; CHECK-NEXT: ) + ;; CHECK-NEXT: ) + ;; CHECK-NEXT: ) + (func $side-effects-in-condition-3 + (local $temp i32) + (if + (local.tee $temp + (global.get $global) ;; the global's value flows into a place that has + ) ;; side effects, so it may be noticed. + (global.set $global (i32.const 1)) ) ) ) +;; As above, but now the global's value flows through multiple layers of +;; things that have no side effects and are safe. (module (memory 1 1) @@ -449,7 +481,7 @@ ;; CHECK: (memory $0 1 1) - ;; CHECK: (func $test + ;; CHECK: (func $side-effects-in-condition-4 ;; CHECK-NEXT: (local $x i32) ;; CHECK-NEXT: (local $y i32) ;; CHECK-NEXT: (if @@ -461,7 +493,10 @@ ;; CHECK-NEXT: (i32.load ;; CHECK-NEXT: (i32.const 2) ;; CHECK-NEXT: ) - ;; CHECK-NEXT: (i32.const 0) + ;; CHECK-NEXT: (i32.add + ;; CHECK-NEXT: (i32.const 0) + ;; CHECK-NEXT: (i32.const 1337) + ;; CHECK-NEXT: ) ;; CHECK-NEXT: ) ;; CHECK-NEXT: ) ;; CHECK-NEXT: (drop @@ -469,11 +504,10 @@ ;; CHECK-NEXT: ) ;; CHECK-NEXT: ) ;; CHECK-NEXT: ) - (func $test + (func $side-effects-in-condition-4 (local $x i32) (local $y i32) (if - ;; As above, but with an extra eqz (i32.eqz (select (local.tee $x @@ -482,7 +516,10 @@ (i32.load (i32.const 2) ) - (global.get $once) + (i32.add + (global.get $once) + (i32.const 1337) + ) ) ) (global.set $once @@ -491,4 +528,3 @@ ) ) ) - From f5ba2c01fe2af04d860c47f132d3f0fe130b502c Mon Sep 17 00:00:00 2001 From: Alon Zakai Date: Tue, 30 Nov 2021 15:59:57 -0800 Subject: [PATCH 5/6] fix --- .../simplify-globals-read_only_to_write.wast | 24 ++++++++++++------- 1 file changed, 16 insertions(+), 8 deletions(-) diff --git a/test/lit/passes/simplify-globals-read_only_to_write.wast b/test/lit/passes/simplify-globals-read_only_to_write.wast index a57b84c3180..2bce9050cb1 100644 --- a/test/lit/passes/simplify-globals-read_only_to_write.wast +++ b/test/lit/passes/simplify-globals-read_only_to_write.wast @@ -323,26 +323,27 @@ (module ;; CHECK: (type $none_=>_none (func)) - ;; CHECK: (global $once i32 (i32.const 0)) + ;; CHECK: (type $i32_=>_i32 (func (param i32) (result i32))) + + ;; CHECK: (global $once (mut i32) (i32.const 0)) (global $once (mut i32) (i32.const 0)) ;; CHECK: (func $clinit ;; CHECK-NEXT: (if - ;; CHECK-NEXT: (block $block (result i32) - ;; CHECK-NEXT: (unreachable) - ;; CHECK-NEXT: (i32.const 0) + ;; CHECK-NEXT: (call $foo + ;; CHECK-NEXT: (global.get $once) ;; CHECK-NEXT: ) ;; CHECK-NEXT: (return) ;; CHECK-NEXT: ) - ;; CHECK-NEXT: (drop + ;; CHECK-NEXT: (global.set $once ;; CHECK-NEXT: (i32.const 1) ;; CHECK-NEXT: ) ;; CHECK-NEXT: ) (func $clinit - ;; As above, but the optimization fails because the if body has effects. + ;; As above, but the optimization fails because the if condition has effects. (if - (block (result i32) - (unreachable) + (call $foo ;; This call may have side effects and it receives the global's + ;; value, which is dangerous. (global.get $once) ) (return) @@ -351,6 +352,13 @@ (i32.const 1) ) ) + + ;; CHECK: (func $foo (param $x i32) (result i32) + ;; CHECK-NEXT: (unreachable) + ;; CHECK-NEXT: ) + (func $foo (param $x i32) (result i32) + (unreachable) + ) ) ;; Using the global's value in a way that can cause side effects prevents the From 61cc6454e63df02754883f8c6422d708123ef392 Mon Sep 17 00:00:00 2001 From: Alon Zakai Date: Tue, 30 Nov 2021 16:00:04 -0800 Subject: [PATCH 6/6] fix --- src/passes/SimplifyGlobals.cpp | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/src/passes/SimplifyGlobals.cpp b/src/passes/SimplifyGlobals.cpp index 933182ac497..9730400d6fe 100644 --- a/src/passes/SimplifyGlobals.cpp +++ b/src/passes/SimplifyGlobals.cpp @@ -126,8 +126,7 @@ struct GlobalUseScanner : public WalkerPass> { return; } - auto global = - readsGlobalOnlyToWriteIt(curr->condition, curr->ifTrue); + auto global = readsGlobalOnlyToWriteIt(curr->condition, curr->ifTrue); if (global.is()) { // This is exactly the pattern we sought! (*infos)[global].readOnlyToWrite++; @@ -155,8 +154,7 @@ struct GlobalUseScanner : public WalkerPass> { // write that global, but also for other reasons. // // Returns the global name if things like up, or a null name otherwise. - Name readsGlobalOnlyToWriteIt(Expression* condition, - Expression* code) { + Name readsGlobalOnlyToWriteIt(Expression* condition, Expression* code) { // See if writing a global is the only effect the code has. (Note that we // don't need to care about the case where the code has no effects at // all - other passes would handle that trivial situation.) @@ -192,12 +190,15 @@ struct GlobalUseScanner : public WalkerPass> { // // To check this, find the get of the global in the condition, and look up // through its parents to see how the global's value is used. - struct FlowScanner : public ExpressionStackWalker> { + struct FlowScanner + : public ExpressionStackWalker> { Name writtenGlobal; PassOptions& passOptions; Module& wasm; - FlowScanner(Name writtenGlobal, PassOptions& passOptions, Module& wasm) : writtenGlobal(writtenGlobal), passOptions(passOptions), wasm(wasm) {} + FlowScanner(Name writtenGlobal, PassOptions& passOptions, Module& wasm) + : writtenGlobal(writtenGlobal), passOptions(passOptions), wasm(wasm) {} bool ok = true; @@ -270,8 +271,7 @@ struct GlobalUseScanner : public WalkerPass> { return; } - auto global = - readsGlobalOnlyToWriteIt(iff->condition, list[1]); + auto global = readsGlobalOnlyToWriteIt(iff->condition, list[1]); if (global.is()) { // This is exactly the pattern we sought! (*infos)[global].readOnlyToWrite++;