diff --git a/Changelog.md b/Changelog.md index 5cb2b7095..f51aaffb2 100644 --- a/Changelog.md +++ b/Changelog.md @@ -6,6 +6,7 @@ Language Features: Compiler Features: * SMTChecker: Properties that are proved safe are now reported explicitly at the end of the analysis. By default, only the number of safe properties is shown. The CLI option ``--model-checker-show-proved-safe`` and the JSON option ``settings.modelChecker.showProvedSafe`` can be enabled to show the full list of safe properties. * SMTChecker: Group all messages about unsupported language features in a single warning. The CLI option ``--model-checker-show-unsupported`` and the JSON option ``settings.modelChecker.showUnsupported`` can be enabled to show the full list. + * Optimizer: Re-implement simplified version of UnusedAssignEliminator and UnusedStoreEliminator. It can correctly remove some unused assignments in deeply nested loops that were ignored by the old version. Bugfixes: diff --git a/libyul/optimiser/UnusedAssignEliminator.cpp b/libyul/optimiser/UnusedAssignEliminator.cpp index 273aa6b79..60021d4a9 100644 --- a/libyul/optimiser/UnusedAssignEliminator.cpp +++ b/libyul/optimiser/UnusedAssignEliminator.cpp @@ -24,48 +24,49 @@ #include #include +#include #include +#include #include #include +#include + using namespace std; using namespace solidity; using namespace solidity::yul; void UnusedAssignEliminator::run(OptimiserStepContext& _context, Block& _ast) { - UnusedAssignEliminator rae{_context.dialect}; - rae(_ast); + UnusedAssignEliminator uae{ + _context.dialect, + ControlFlowSideEffectsCollector{_context.dialect, _ast}.functionSideEffectsNamed() + }; + uae(_ast); - StatementRemover remover{rae.m_pendingRemovals}; + uae.m_storesToRemove += uae.m_allStores - uae.m_usedStores; + + set toRemove{uae.m_storesToRemove.begin(), uae.m_storesToRemove.end()}; + StatementRemover remover{toRemove}; remover(_ast); } void UnusedAssignEliminator::operator()(Identifier const& _identifier) { - changeUndecidedTo(_identifier.name, State::Used); -} - -void UnusedAssignEliminator::operator()(VariableDeclaration const& _variableDeclaration) -{ - UnusedStoreBase::operator()(_variableDeclaration); - - for (auto const& var: _variableDeclaration.variables) - m_declaredVariables.emplace(var.name); + markUsed(_identifier.name); } void UnusedAssignEliminator::operator()(Assignment const& _assignment) { visit(*_assignment.value); - for (auto const& var: _assignment.variableNames) - changeUndecidedTo(var.name, State::Unused); + // Do not visit the variables because they are Identifiers } + void UnusedAssignEliminator::operator()(FunctionDefinition const& _functionDefinition) { - ScopedSaveAndRestore outerDeclaredVariables(m_declaredVariables, {}); ScopedSaveAndRestore outerReturnVariables(m_returnVariables, {}); for (auto const& retParam: _functionDefinition.returnVariables) @@ -74,20 +75,37 @@ void UnusedAssignEliminator::operator()(FunctionDefinition const& _functionDefin UnusedStoreBase::operator()(_functionDefinition); } +void UnusedAssignEliminator::operator()(FunctionCall const& _functionCall) +{ + UnusedStoreBase::operator()(_functionCall); + + ControlFlowSideEffects sideEffects; + if (auto builtin = m_dialect.builtin(_functionCall.functionName.name)) + sideEffects = builtin->controlFlowSideEffects; + else + sideEffects = m_controlFlowSideEffects.at(_functionCall.functionName.name); + + if (!sideEffects.canContinue) + // We do not return from the current function, so it is OK to also + // clear the return variables. + m_activeStores.clear(); +} + void UnusedAssignEliminator::operator()(Leave const&) { for (YulString name: m_returnVariables) - changeUndecidedTo(name, State::Used); + markUsed(name); + m_activeStores.clear(); } void UnusedAssignEliminator::operator()(Block const& _block) { - ScopedSaveAndRestore outerDeclaredVariables(m_declaredVariables, {}); - UnusedStoreBase::operator()(_block); - for (auto const& var: m_declaredVariables) - finalize(var, State::Unused); + for (auto const& statement: _block.statements) + if (auto const* varDecl = get_if(&statement)) + for (auto const& var: varDecl->variables) + m_activeStores.erase(var.name); } void UnusedAssignEliminator::visit(Statement const& _statement) @@ -95,63 +113,49 @@ void UnusedAssignEliminator::visit(Statement const& _statement) UnusedStoreBase::visit(_statement); if (auto const* assignment = get_if(&_statement)) - if (assignment->variableNames.size() == 1) - // Default-construct it in "Undecided" state if it does not yet exist. - m_stores[assignment->variableNames.front().name][&_statement]; + { + // We do not remove assignments whose values might have side-effects, + // but clear the active stores to the assigned variables in any case. + if (SideEffectsCollector{m_dialect, *assignment->value}.movable()) + { + m_allStores.insert(&_statement); + for (auto const& var: assignment->variableNames) + m_activeStores[var.name] = {&_statement}; + } + else + for (auto const& var: assignment->variableNames) + m_activeStores[var.name].clear(); + } } -void UnusedAssignEliminator::shortcutNestedLoop(TrackedStores const& _zeroRuns) +void UnusedAssignEliminator::shortcutNestedLoop(ActiveStores const& _zeroRuns) { // Shortcut to avoid horrible runtime: // Change all assignments that were newly introduced in the for loop to "used". // We do not have to do that with the "break" or "continue" paths, because // they will be joined later anyway. - // TODO parallel traversal might be more efficient here. - for (auto& [variable, stores]: m_stores) + + for (auto& [variable, stores]: m_activeStores) + { + auto zeroIt = _zeroRuns.find(variable); for (auto& assignment: stores) { - auto zeroIt = _zeroRuns.find(variable); - if (zeroIt != _zeroRuns.end() && zeroIt->second.count(assignment.first)) + if (zeroIt != _zeroRuns.end() && zeroIt->second.count(assignment)) continue; - assignment.second = State::Value::Used; + m_usedStores.insert(assignment); } + } } void UnusedAssignEliminator::finalizeFunctionDefinition(FunctionDefinition const& _functionDefinition) { - for (auto const& param: _functionDefinition.parameters) - finalize(param.name, State::Unused); for (auto const& retParam: _functionDefinition.returnVariables) - finalize(retParam.name, State::Used); + markUsed(retParam.name); } -void UnusedAssignEliminator::changeUndecidedTo(YulString _variable, UnusedAssignEliminator::State _newState) +void UnusedAssignEliminator::markUsed(YulString _variable) { - for (auto& assignment: m_stores[_variable]) - if (assignment.second == State::Undecided) - assignment.second = _newState; -} - -void UnusedAssignEliminator::finalize(YulString _variable, UnusedAssignEliminator::State _finalState) -{ - std::map stores = std::move(m_stores[_variable]); - m_stores.erase(_variable); - - for (auto& breakAssignments: m_forLoopInfo.pendingBreakStmts) - { - util::joinMap(stores, std::move(breakAssignments[_variable]), State::join); - breakAssignments.erase(_variable); - } - for (auto& continueAssignments: m_forLoopInfo.pendingContinueStmts) - { - util::joinMap(stores, std::move(continueAssignments[_variable]), State::join); - continueAssignments.erase(_variable); - } - - for (auto&& [statement, state]: stores) - if ( - (state == State::Unused || (state == State::Undecided && _finalState == State::Unused)) && - SideEffectsCollector{m_dialect, *std::get(*statement).value}.movable() - ) - m_pendingRemovals.insert(statement); + for (auto& assignment: m_activeStores[_variable]) + m_usedStores.insert(assignment); + m_activeStores.erase(_variable); } diff --git a/libyul/optimiser/UnusedAssignEliminator.h b/libyul/optimiser/UnusedAssignEliminator.h index b5a2998a1..800d626eb 100644 --- a/libyul/optimiser/UnusedAssignEliminator.h +++ b/libyul/optimiser/UnusedAssignEliminator.h @@ -26,6 +26,7 @@ #include #include #include +#include #include #include @@ -62,28 +63,34 @@ struct Dialect; * Detailed rules: * * The AST is traversed twice: in an information gathering step and in the - * actual removal step. During information gathering, we maintain a - * mapping from assignment statements to the three states - * "unused", "undecided" and "used". - * When an assignment is visited, it is added to the mapping in the "undecided" state - * (see remark about for loops below) and every other assignment to the same variable - * that is still in the "undecided" state is changed to "unused". - * When a variable is referenced, the state of any assignment to that variable still - * in the "undecided" state is changed to "used". - * At points where control flow splits, a copy - * of the mapping is handed over to each branch. At points where control flow - * joins, the two mappings coming from the two branches are combined in the following way: - * Statements that are only in one mapping or have the same state are used unchanged. - * Conflicting values are resolved in the following way: - * "unused", "undecided" -> "undecided" - * "unused", "used" -> "used" - * "undecided, "used" -> "used". + * actual removal step. During information gathering, assignment statements + * can be marked as "potentially unused" or as "used". + * + * When an assignment is visited, it is stored in the "set of all stores" and + * added to the branch-dependent "active" sets for the assigned variables. This active + * set for a variable contains all statements where that variable was last assigned to, i.e. + * where a read from that variable could read from. + * Furthermore, all other active sets for the assigned variables are cleared. + * + * When a reference to a variable is visited, the active assignments to that variable + * in the current branch are marked as "used". This mark is permanent. + * Also, the active set for this variable in the current branch is cleared. + * + * At points where control-flow splits, we maintain a copy of the active set + * (all other data structures are shared across branches). + * + * At control-flow joins, we combine the sets of active stores for each variable. + * + * In the example above, the active set right after the assignment "b := mload(a)" (but before + * the control-flow join) is "b := mload(a)"; the assignment "b := 2" was removed. + * After the control-flow join it will contain both "b := mload(a)" and "b := 2", coming from + * the two branches. * * For for-loops, the condition, body and post-part are visited twice, taking * the joining control-flow at the condition into account. * In other words, we create three control flow paths: Zero runs of the loop, * one run and two runs and then combine them at the end. - * Running at most twice is enough because there are only three different states. + * Running at most twice is enough because this takes into account all possible control-flow connections. * * Since this algorithm has exponential runtime in the nesting depth of for loops, * a shortcut is taken at a certain nesting level: We only use the zero- and @@ -93,14 +100,13 @@ struct Dialect; * For switch statements that have a "default"-case, there is no control-flow * part that skips the switch. * - * At ``leave`` statements, all return variables are set to "used". + * At ``leave`` statements, all return variables are set to "used" and the set of active statements + * is cleared. * - * When a variable goes out of scope, all statements still in the "undecided" - * state are changed to "unused", unless the variable is the return - * parameter of a function - there, the state changes to "used". - * - * In the second traversal, all assignments that are in the "unused" state are removed. + * If a function or builtin is called that does not continue, the set of active statements is + * cleared for all variables. * + * In the second traversal, all assignments that are not marked as "used" are removed. * * This step is usually run right after the SSA transform to complete * the generation of the pseudo-SSA. @@ -113,12 +119,18 @@ public: static constexpr char const* name{"UnusedAssignEliminator"}; static void run(OptimiserStepContext&, Block& _ast); - explicit UnusedAssignEliminator(Dialect const& _dialect): UnusedStoreBase(_dialect) {} + explicit UnusedAssignEliminator( + Dialect const& _dialect, + std::map _controlFlowSideEffects + ): + UnusedStoreBase(_dialect), + m_controlFlowSideEffects(_controlFlowSideEffects) + {} void operator()(Identifier const& _identifier) override; - void operator()(VariableDeclaration const& _variableDeclaration) override; void operator()(Assignment const& _assignment) override; void operator()(FunctionDefinition const&) override; + void operator()(FunctionCall const& _functionCall) override; void operator()(Leave const&) override; void operator()(Block const& _block) override; @@ -126,18 +138,13 @@ public: void visit(Statement const& _statement) override; private: - void shortcutNestedLoop(TrackedStores const& _beforeLoop) override; + void shortcutNestedLoop(ActiveStores const& _beforeLoop) override; void finalizeFunctionDefinition(FunctionDefinition const& _functionDefinition) override; - void changeUndecidedTo(YulString _variable, State _newState); - /// Called when a variable goes out of scope. Sets the state of all still undecided - /// assignments to the final state. In this case, this also applies to pending - /// break and continue TrackedStores. - void finalize(YulString _variable, State _finalState); + void markUsed(YulString _variable); - - std::set m_declaredVariables; std::set m_returnVariables; + std::map m_controlFlowSideEffects; }; } diff --git a/libyul/optimiser/UnusedStoreBase.cpp b/libyul/optimiser/UnusedStoreBase.cpp index 49f550842..c31fda89c 100644 --- a/libyul/optimiser/UnusedStoreBase.cpp +++ b/libyul/optimiser/UnusedStoreBase.cpp @@ -37,47 +37,50 @@ void UnusedStoreBase::operator()(If const& _if) { visit(*_if.condition); - TrackedStores skipBranch{m_stores}; + ActiveStores skipBranch{m_activeStores}; (*this)(_if.body); - merge(m_stores, std::move(skipBranch)); + merge(m_activeStores, std::move(skipBranch)); } void UnusedStoreBase::operator()(Switch const& _switch) { visit(*_switch.expression); - TrackedStores const preState{m_stores}; + ActiveStores const preState{m_activeStores}; bool hasDefault = false; - vector branches; + vector branches; for (auto const& c: _switch.cases) { if (!c.value) hasDefault = true; (*this)(c.body); - branches.emplace_back(std::move(m_stores)); - m_stores = preState; + branches.emplace_back(std::move(m_activeStores)); + m_activeStores = preState; } if (hasDefault) { - m_stores = std::move(branches.back()); + m_activeStores = std::move(branches.back()); branches.pop_back(); } for (auto& branch: branches) - merge(m_stores, std::move(branch)); + merge(m_activeStores, std::move(branch)); } void UnusedStoreBase::operator()(FunctionDefinition const& _functionDefinition) { - ScopedSaveAndRestore outerAssignments(m_stores, {}); + ScopedSaveAndRestore allStores(m_allStores, {}); + ScopedSaveAndRestore usedStores(m_usedStores, {}); + ScopedSaveAndRestore outerAssignments(m_activeStores, {}); ScopedSaveAndRestore forLoopInfo(m_forLoopInfo, {}); ScopedSaveAndRestore forLoopNestingDepth(m_forLoopNestingDepth, 0); (*this)(_functionDefinition.body); finalizeFunctionDefinition(_functionDefinition); + m_storesToRemove += m_allStores - m_usedStores; } void UnusedStoreBase::operator()(ForLoop const& _forLoop) @@ -94,10 +97,10 @@ void UnusedStoreBase::operator()(ForLoop const& _forLoop) visit(*_forLoop.condition); - TrackedStores zeroRuns{m_stores}; + ActiveStores zeroRuns{m_activeStores}; (*this)(_forLoop.body); - merge(m_stores, std::move(m_forLoopInfo.pendingContinueStmts)); + merge(m_activeStores, std::move(m_forLoopInfo.pendingContinueStmts)); m_forLoopInfo.pendingContinueStmts = {}; (*this)(_forLoop.post); @@ -106,54 +109,54 @@ void UnusedStoreBase::operator()(ForLoop const& _forLoop) if (m_forLoopNestingDepth < 6) { // Do the second run only for small nesting depths to avoid horrible runtime. - TrackedStores oneRun{m_stores}; + ActiveStores oneRun{m_activeStores}; (*this)(_forLoop.body); - merge(m_stores, std::move(m_forLoopInfo.pendingContinueStmts)); + merge(m_activeStores, std::move(m_forLoopInfo.pendingContinueStmts)); m_forLoopInfo.pendingContinueStmts.clear(); (*this)(_forLoop.post); visit(*_forLoop.condition); // Order of merging does not matter because "max" is commutative and associative. - merge(m_stores, std::move(oneRun)); + merge(m_activeStores, std::move(oneRun)); } else // Shortcut to avoid horrible runtime. shortcutNestedLoop(zeroRuns); // Order of merging does not matter because "max" is commutative and associative. - merge(m_stores, std::move(zeroRuns)); - merge(m_stores, std::move(m_forLoopInfo.pendingBreakStmts)); + merge(m_activeStores, std::move(zeroRuns)); + merge(m_activeStores, std::move(m_forLoopInfo.pendingBreakStmts)); m_forLoopInfo.pendingBreakStmts.clear(); } void UnusedStoreBase::operator()(Break const&) { - m_forLoopInfo.pendingBreakStmts.emplace_back(std::move(m_stores)); - m_stores.clear(); + m_forLoopInfo.pendingBreakStmts.emplace_back(std::move(m_activeStores)); + m_activeStores.clear(); } void UnusedStoreBase::operator()(Continue const&) { - m_forLoopInfo.pendingContinueStmts.emplace_back(std::move(m_stores)); - m_stores.clear(); + m_forLoopInfo.pendingContinueStmts.emplace_back(std::move(m_activeStores)); + m_activeStores.clear(); } -void UnusedStoreBase::merge(TrackedStores& _target, TrackedStores&& _other) +void UnusedStoreBase::merge(ActiveStores& _target, ActiveStores&& _other) { util::joinMap(_target, std::move(_other), []( - map& _assignmentHere, - map&& _assignmentThere + set& _storesHere, + set&& _storesThere ) { - return util::joinMap(_assignmentHere, std::move(_assignmentThere), State::join); + _storesHere += _storesThere; }); } -void UnusedStoreBase::merge(TrackedStores& _target, vector&& _source) +void UnusedStoreBase::merge(ActiveStores& _target, vector&& _source) { - for (TrackedStores& ts: _source) + for (ActiveStores& ts: _source) merge(_target, std::move(ts)); _source.clear(); } diff --git a/libyul/optimiser/UnusedStoreBase.h b/libyul/optimiser/UnusedStoreBase.h index 15dccb04a..0d618d8d2 100644 --- a/libyul/optimiser/UnusedStoreBase.h +++ b/libyul/optimiser/UnusedStoreBase.h @@ -38,8 +38,11 @@ struct Dialect; * * The class tracks the state of abstract "stores" (assignments or mstore/sstore * statements) across the control-flow. It is the job of the derived class to create - * the stores and track references, but the base class adjusts their "used state" at - * control-flow splits and joins. + * the stores and track references, but the base class manages control-flow splits and joins. + * + * In general, active stores are those where it has not yet been determined if they are used + * or not. Those are split and joined at control-flow forks. Once a store has been deemed + * used, it is removed from the active set and marked as used and this will never change. * * Prerequisite: Disambiguator, ForLoopInitRewriter. */ @@ -57,28 +60,12 @@ public: void operator()(Continue const&) override; protected: - class State - { - public: - enum Value { Unused, Undecided, Used }; - State(Value _value = Undecided): m_value(_value) {} - inline bool operator==(State _other) const { return m_value == _other.m_value; } - inline bool operator!=(State _other) const { return !operator==(_other); } - static inline void join(State& _a, State const& _b) - { - // Using "max" works here because of the order of the values in the enum. - _a.m_value = Value(std::max(int(_a.m_value), int(_b.m_value))); - } - private: - Value m_value = Undecided; - }; - - using TrackedStores = std::map>; + using ActiveStores = std::map>; /// This function is called for a loop that is nested too deep to avoid /// horrible runtime and should just resolve the situation in a pragmatic /// and correct manner. - virtual void shortcutNestedLoop(TrackedStores const& _beforeLoop) = 0; + virtual void shortcutNestedLoop(ActiveStores const& _beforeLoop) = 0; /// This function is called right before the scoped restore of the function definition. virtual void finalizeFunctionDefinition(FunctionDefinition const& /*_functionDefinition*/) {} @@ -86,20 +73,26 @@ protected: /// Joins the assignment mapping of @a _source into @a _target according to the rules laid out /// above. /// Will destroy @a _source. - static void merge(TrackedStores& _target, TrackedStores&& _source); - static void merge(TrackedStores& _target, std::vector&& _source); + static void merge(ActiveStores& _target, ActiveStores&& _source); + static void merge(ActiveStores& _target, std::vector&& _source); Dialect const& m_dialect; - std::set m_pendingRemovals; - TrackedStores m_stores; + /// Set of all stores encountered during the traversal (in the current function). + std::set m_allStores; + /// Set of stores that are marked as being used (in the current function). + std::set m_usedStores; + /// List of stores that can be removed (globally). + std::vector m_storesToRemove; + /// Active (undecided) stores in the current branch. + ActiveStores m_activeStores; /// Working data for traversing for-loops. struct ForLoopInfo { /// Tracked assignment states for each break statement. - std::vector pendingBreakStmts; + std::vector pendingBreakStmts; /// Tracked assignment states for each continue statement. - std::vector pendingContinueStmts; + std::vector pendingContinueStmts; }; ForLoopInfo m_forLoopInfo; size_t m_forLoopNestingDepth = 0; diff --git a/libyul/optimiser/UnusedStoreEliminator.cpp b/libyul/optimiser/UnusedStoreEliminator.cpp index 3df9c6319..2c3d03e4e 100644 --- a/libyul/optimiser/UnusedStoreEliminator.cpp +++ b/libyul/optimiser/UnusedStoreEliminator.cpp @@ -78,17 +78,17 @@ void UnusedStoreEliminator::run(OptimiserStepContext& _context, Block& _ast) ignoreMemory }; rse(_ast); - if ( - auto evmDialect = dynamic_cast(&_context.dialect); - evmDialect && evmDialect->providesObjectAccess() - ) - rse.changeUndecidedTo(State::Unused, Location::Memory); - else - rse.changeUndecidedTo(State::Used, Location::Memory); - rse.changeUndecidedTo(State::Used, Location::Storage); - rse.scheduleUnusedForDeletion(); - StatementRemover remover(rse.m_pendingRemovals); + auto evmDialect = dynamic_cast(&_context.dialect); + if (evmDialect && evmDialect->providesObjectAccess()) + rse.clearActive(Location::Memory); + else + rse.markActiveAsUsed(Location::Memory); + rse.markActiveAsUsed(Location::Storage); + rse.m_storesToRemove += rse.m_allStores - rse.m_usedStores; + + set toRemove{rse.m_storesToRemove.begin(), rse.m_storesToRemove.end()}; + StatementRemover remover{toRemove}; remover(_ast); } @@ -121,12 +121,12 @@ void UnusedStoreEliminator::operator()(FunctionCall const& _functionCall) sideEffects = m_controlFlowSideEffects.at(_functionCall.functionName.name); if (sideEffects.canTerminate) - changeUndecidedTo(State::Used, Location::Storage); + markActiveAsUsed(Location::Storage); if (!sideEffects.canContinue) { - changeUndecidedTo(State::Unused, Location::Memory); + clearActive(Location::Memory); if (!sideEffects.canTerminate) - changeUndecidedTo(State::Unused, Location::Storage); + clearActive(Location::Storage); } } @@ -139,7 +139,7 @@ void UnusedStoreEliminator::operator()(FunctionDefinition const& _functionDefini void UnusedStoreEliminator::operator()(Leave const&) { - changeUndecidedTo(State::Used); + markActiveAsUsed(); } void UnusedStoreEliminator::visit(Statement const& _statement) @@ -183,10 +183,14 @@ void UnusedStoreEliminator::visit(Statement const& _statement) yulAssert(isCandidateForRemoval == (isStorageWrite || (!m_ignoreMemory && isMemoryWrite))); if (isCandidateForRemoval) { - State initialState = State::Undecided; if (*instruction == Instruction::RETURNDATACOPY) { - initialState = State::Used; + // Out-of-bounds access to the returndata buffer results in a revert, + // so we are careful not to remove a potentially reverting call to a builtin. + // The only way the Solidity compiler uses `returndatacopy` is + // `returndatacopy(X, 0, returndatasize())`, so we only allow to remove this pattern + // (which is guaranteed to never cause an out-of-bounds revert). + bool allowReturndatacopyToBeRemoved = false; auto startOffset = identifierNameIfSSA(funCall->arguments.at(1)); auto length = identifierNameIfSSA(funCall->arguments.at(2)); if (length && startOffset) @@ -197,22 +201,22 @@ void UnusedStoreEliminator::visit(Statement const& _statement) lengthCall && toEVMInstruction(m_dialect, lengthCall->functionName.name) == Instruction::RETURNDATASIZE ) - initialState = State::Undecided; + allowReturndatacopyToBeRemoved = true; } + if (!allowReturndatacopyToBeRemoved) + return; } - m_stores[YulString{}].insert({&_statement, initialState}); + m_allStores.insert(&_statement); vector operations = operationsFromFunctionCall(*funCall); yulAssert(operations.size() == 1, ""); + if (operations.front().location == Location::Storage) + activeStorageStores().insert(&_statement); + else + activeMemoryStores().insert(&_statement); m_storeOperations[&_statement] = std::move(operations.front()); } } -void UnusedStoreEliminator::finalizeFunctionDefinition(FunctionDefinition const&) -{ - changeUndecidedTo(State::Used); - scheduleUnusedForDeletion(); -} - vector UnusedStoreEliminator::operationsFromFunctionCall( FunctionCall const& _functionCall ) const @@ -265,15 +269,28 @@ vector UnusedStoreEliminator::operationsFromFu void UnusedStoreEliminator::applyOperation(UnusedStoreEliminator::Operation const& _operation) { - for (auto& [statement, state]: m_stores[YulString{}]) - if (state == State::Undecided) + set& active = + _operation.location == Location::Storage ? + activeStorageStores() : + activeMemoryStores(); + + + for (auto it = active.begin(); it != active.end();) + { + Statement const* statement = *it; + Operation const& storeOperation = m_storeOperations.at(statement); + if (_operation.effect == Effect::Read && !knownUnrelated(storeOperation, _operation)) { - Operation const& storeOperation = m_storeOperations.at(statement); - if (_operation.effect == Effect::Read && !knownUnrelated(storeOperation, _operation)) - state = State::Used; - else if (_operation.effect == Effect::Write && knownCovered(storeOperation, _operation)) - state = State::Unused; + // This store is read from, mark it as used and remove it from the active set. + m_usedStores.insert(statement); + it = active.erase(it); } + else if (_operation.effect == Effect::Write && knownCovered(storeOperation, _operation)) + // This store is overwritten before being read, remove it from the active set. + it = active.erase(it); + else + ++it; + } } bool UnusedStoreEliminator::knownUnrelated( @@ -390,16 +407,27 @@ bool UnusedStoreEliminator::knownCovered( return false; } -void UnusedStoreEliminator::changeUndecidedTo( - State _newState, - optional _onlyLocation) +void UnusedStoreEliminator::markActiveAsUsed( + optional _onlyLocation +) { - for (auto& [statement, state]: m_stores[YulString{}]) - if ( - state == State::Undecided && - (_onlyLocation == nullopt || *_onlyLocation == m_storeOperations.at(statement).location) - ) - state = _newState; + if (_onlyLocation == nullopt || _onlyLocation == Location::Memory) + for (Statement const* statement: activeMemoryStores()) + m_usedStores.insert(statement); + if (_onlyLocation == nullopt || _onlyLocation == Location::Storage) + for (Statement const* statement: activeStorageStores()) + m_usedStores.insert(statement); + clearActive(_onlyLocation); +} + +void UnusedStoreEliminator::clearActive( + optional _onlyLocation +) +{ + if (_onlyLocation == nullopt || _onlyLocation == Location::Memory) + activeMemoryStores() = {}; + if (_onlyLocation == nullopt || _onlyLocation == Location::Storage) + activeStorageStores() = {}; } optional UnusedStoreEliminator::identifierNameIfSSA(Expression const& _expression) const @@ -409,10 +437,3 @@ optional UnusedStoreEliminator::identifierNameIfSSA(Expression const& return {identifier->name}; return nullopt; } - -void UnusedStoreEliminator::scheduleUnusedForDeletion() -{ - for (auto const& [statement, state]: m_stores[YulString{}]) - if (state == State::Unused) - m_pendingRemovals.insert(statement); -} diff --git a/libyul/optimiser/UnusedStoreEliminator.h b/libyul/optimiser/UnusedStoreEliminator.h index 7fbf9885f..89dbabd78 100644 --- a/libyul/optimiser/UnusedStoreEliminator.h +++ b/libyul/optimiser/UnusedStoreEliminator.h @@ -50,8 +50,7 @@ struct AssignedValue; * to sstore, as we don't know whether the memory location will be read once we leave the function's scope, * so the statement will be removed only if all code code paths lead to a memory overwrite. * - * The m_store member of UnusedStoreBase is only used with the empty yul string - * as key in the first dimension. + * The m_store member of UnusedStoreBase uses the key "m" for memory and "s" for storage stores. * * Best run in SSA form. * @@ -93,20 +92,26 @@ public: }; private: - void shortcutNestedLoop(TrackedStores const&) override + std::set& activeMemoryStores() { return m_activeStores["m"_yulstring]; } + std::set& activeStorageStores() { return m_activeStores["s"_yulstring]; } + + void shortcutNestedLoop(ActiveStores const&) override { // We might only need to do this for newly introduced stores in the loop. - changeUndecidedTo(State::Used); + markActiveAsUsed(); + } + void finalizeFunctionDefinition(FunctionDefinition const&) override + { + markActiveAsUsed(); } - void finalizeFunctionDefinition(FunctionDefinition const&) override; std::vector operationsFromFunctionCall(FunctionCall const& _functionCall) const; void applyOperation(Operation const& _operation); bool knownUnrelated(Operation const& _op1, Operation const& _op2) const; bool knownCovered(Operation const& _covered, Operation const& _covering) const; - void changeUndecidedTo(State _newState, std::optional _onlyLocation = std::nullopt); - void scheduleUnusedForDeletion(); + void markActiveAsUsed(std::optional _onlyLocation = std::nullopt); + void clearActive(std::optional _onlyLocation = std::nullopt); std::optional identifierNameIfSSA(Expression const& _expression) const; diff --git a/test/libsolidity/semanticTests/externalContracts/deposit_contract.sol b/test/libsolidity/semanticTests/externalContracts/deposit_contract.sol index 389661fd7..48f7247a6 100644 --- a/test/libsolidity/semanticTests/externalContracts/deposit_contract.sol +++ b/test/libsolidity/semanticTests/externalContracts/deposit_contract.sol @@ -176,7 +176,7 @@ contract DepositContract is IDepositContract, ERC165 { } // ---- // constructor() -// gas irOptimized: 1430741 +// gas irOptimized: 1419712 // gas legacy: 2427905 // gas legacyOptimized: 1773081 // supportsInterface(bytes4): 0x0 -> 0 diff --git a/test/libyul/yulOptimizerTests/unusedAssignEliminator/conditionally_assign_before_conditional_revert.yul b/test/libyul/yulOptimizerTests/unusedAssignEliminator/conditionally_assign_before_conditional_revert.yul new file mode 100644 index 000000000..3eeea6f4d --- /dev/null +++ b/test/libyul/yulOptimizerTests/unusedAssignEliminator/conditionally_assign_before_conditional_revert.yul @@ -0,0 +1,33 @@ +{ + function g() { + if calldataload(10) { revert(0, 0) } + } + function f() { + let a := calldataload(0) + if calldataload(1) { + // this can NOT be removed + a := 2 + g() + } + sstore(0, a) + } +} +// ---- +// step: unusedAssignEliminator +// +// { +// function g() +// { +// if calldataload(10) { revert(0, 0) } +// } +// function f() +// { +// let a := calldataload(0) +// if calldataload(1) +// { +// a := 2 +// g() +// } +// sstore(0, a) +// } +// } diff --git a/test/libyul/yulOptimizerTests/unusedAssignEliminator/conditionally_assign_before_leave.yul b/test/libyul/yulOptimizerTests/unusedAssignEliminator/conditionally_assign_before_leave.yul new file mode 100644 index 000000000..4564f5d07 --- /dev/null +++ b/test/libyul/yulOptimizerTests/unusedAssignEliminator/conditionally_assign_before_leave.yul @@ -0,0 +1,22 @@ +{ + function f() { + let a := calldataload(0) + if calldataload(1) { + // this can be removed + a := 2 + leave + } + sstore(0, a) + } +} +// ---- +// step: unusedAssignEliminator +// +// { +// function f() +// { +// let a := calldataload(0) +// if calldataload(1) { leave } +// sstore(0, a) +// } +// } diff --git a/test/libyul/yulOptimizerTests/unusedAssignEliminator/conditionally_assign_before_revert.yul b/test/libyul/yulOptimizerTests/unusedAssignEliminator/conditionally_assign_before_revert.yul new file mode 100644 index 000000000..469da01fc --- /dev/null +++ b/test/libyul/yulOptimizerTests/unusedAssignEliminator/conditionally_assign_before_revert.yul @@ -0,0 +1,17 @@ +{ + let a := calldataload(0) + if calldataload(1) { + // this can be removed + a := 2 + revert(0, 0) + } + sstore(0, a) +} +// ---- +// step: unusedAssignEliminator +// +// { +// let a := calldataload(0) +// if calldataload(1) { revert(0, 0) } +// sstore(0, a) +// } diff --git a/test/libyul/yulOptimizerTests/unusedAssignEliminator/for_deep_noremove.yul b/test/libyul/yulOptimizerTests/unusedAssignEliminator/for_deep_noremove.yul index 02a735f9e..c0a84c245 100644 --- a/test/libyul/yulOptimizerTests/unusedAssignEliminator/for_deep_noremove.yul +++ b/test/libyul/yulOptimizerTests/unusedAssignEliminator/for_deep_noremove.yul @@ -46,10 +46,7 @@ // for { } 1 { } // { // for { } 1 { a := 10 } -// { -// b := 12 -// b := 11 -// } +// { b := 11 } // } // } // } diff --git a/test/libyul/yulOptimizerTests/unusedAssignEliminator/revert_after_assign_return.yul b/test/libyul/yulOptimizerTests/unusedAssignEliminator/revert_after_assign_return.yul new file mode 100644 index 000000000..0a8a3582f --- /dev/null +++ b/test/libyul/yulOptimizerTests/unusedAssignEliminator/revert_after_assign_return.yul @@ -0,0 +1,35 @@ +{ + function g() -> x { + x := 7 + if calldataload(0) { + x := 3 + reverting() + } + if calldataload(1) { + x := 3 + leave + } + x := 2 + reverting() + } + function reverting() { revert(0, 0) } + sstore(0, g()) +} +// ---- +// step: unusedAssignEliminator +// +// { +// function g() -> x +// { +// if calldataload(0) { reverting() } +// if calldataload(1) +// { +// x := 3 +// leave +// } +// reverting() +// } +// function reverting() +// { revert(0, 0) } +// sstore(0, g()) +// }