Re-implement unused assign / unused store eliminator.

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:

libyul/optimiser/UnusedAssignEliminator.cpp

@@ -24,48 +24,49 @@
 
 #include <libyul/optimiser/Semantics.h>
 #include <libyul/optimiser/OptimizerUtilities.h>
+#include <libyul/ControlFlowSideEffectsCollector.h>
 #include <libyul/AST.h>
+#include <libyul/AsmPrinter.h>
 
 #include <libsolutil/CommonData.h>
 
 #include <range/v3/action/remove_if.hpp>
 
+#include <iostream>
+
 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<Statement const*> 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<VariableDeclaration>(&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<Assignment>(&_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<Statement const*, State> 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<Assignment>(*statement).value}.movable()
-		)
-			m_pendingRemovals.insert(statement);
+	for (auto& assignment: m_activeStores[_variable])
+		m_usedStores.insert(assignment);
+	m_activeStores.erase(_variable);
 }

libyul/optimiser/UnusedAssignEliminator.h

@@ -26,6 +26,7 @@
 #include <libyul/optimiser/ASTWalker.h>
 #include <libyul/optimiser/OptimiserStep.h>
 #include <libyul/optimiser/UnusedStoreBase.h>
+#include <libyul/optimiser/Semantics.h>
 
 #include <map>
 #include <vector>
@@ -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<YulString, ControlFlowSideEffects> _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<YulString> m_declaredVariables;
 	std::set<YulString> m_returnVariables;
+	std::map<YulString, ControlFlowSideEffects> m_controlFlowSideEffects;
 };
 
 }

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<TrackedStores> branches;
+	vector<ActiveStores> 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<Statement const*, State>& _assignmentHere,
-		map<Statement const*, State>&& _assignmentThere
+		set<Statement const*>& _storesHere,
+		set<Statement const*>&& _storesThere
 	)
 	{
-		return util::joinMap(_assignmentHere, std::move(_assignmentThere), State::join);
+		_storesHere += _storesThere;
 	});
 }
 
-void UnusedStoreBase::merge(TrackedStores& _target, vector<TrackedStores>&& _source)
+void UnusedStoreBase::merge(ActiveStores& _target, vector<ActiveStores>&& _source)
 {
-	for (TrackedStores& ts: _source)
+	for (ActiveStores& ts: _source)
 		merge(_target, std::move(ts));
 	_source.clear();
 }

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<YulString, std::map<Statement const*, State>>;
+	using ActiveStores = std::map<YulString, std::set<Statement const*>>;
 
 	/// 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<TrackedStores>&& _source);
+	static void merge(ActiveStores& _target, ActiveStores&& _source);
+	static void merge(ActiveStores& _target, std::vector<ActiveStores>&& _source);
 
 	Dialect const& m_dialect;
-	std::set<Statement const*> m_pendingRemovals;
-	TrackedStores m_stores;
+	/// Set of all stores encountered during the traversal (in the current function).
+	std::set<Statement const*> m_allStores;
+	/// Set of stores that are marked as being used (in the current function).
+	std::set<Statement const*> m_usedStores;
+	/// List of stores that can be removed (globally).
+	std::vector<Statement const*> 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<TrackedStores> pendingBreakStmts;
+		std::vector<ActiveStores> pendingBreakStmts;
 		/// Tracked assignment states for each continue statement.
-		std::vector<TrackedStores> pendingContinueStmts;
+		std::vector<ActiveStores> pendingContinueStmts;
 	};
 	ForLoopInfo m_forLoopInfo;
 	size_t m_forLoopNestingDepth = 0;

libyul/optimiser/UnusedStoreEliminator.cpp

@@ -78,17 +78,17 @@ void UnusedStoreEliminator::run(OptimiserStepContext& _context, Block& _ast)
 		ignoreMemory
 	};
 	rse(_ast);
-	if (
-		auto evmDialect = dynamic_cast<EVMDialect const*>(&_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<EVMDialect const*>(&_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<Statement const*> 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<Operation> 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::Operation> UnusedStoreEliminator::operationsFromFunctionCall(
 	FunctionCall const& _functionCall
 ) const
@@ -265,15 +269,28 @@ vector<UnusedStoreEliminator::Operation> UnusedStoreEliminator::operationsFromFu
 
 void UnusedStoreEliminator::applyOperation(UnusedStoreEliminator::Operation const& _operation)
 {
-	for (auto& [statement, state]: m_stores[YulString{}])
-		if (state == State::Undecided)
+	set<Statement const*>& 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<UnusedStoreEliminator::Location> _onlyLocation)
+void UnusedStoreEliminator::markActiveAsUsed(
+	optional<UnusedStoreEliminator::Location> _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<UnusedStoreEliminator::Location> _onlyLocation
+)
+{
+	if (_onlyLocation == nullopt || _onlyLocation == Location::Memory)
+		activeMemoryStores() = {};
+	if (_onlyLocation == nullopt || _onlyLocation == Location::Storage)
+		activeStorageStores() = {};
 }
 
 optional<YulString> UnusedStoreEliminator::identifierNameIfSSA(Expression const& _expression) const
@@ -409,10 +437,3 @@ optional<YulString> 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);
-}

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<Statement const*>& activeMemoryStores() { return m_activeStores["m"_yulstring]; }
+	std::set<Statement const*>& 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<Operation> 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<Location> _onlyLocation = std::nullopt);
-	void scheduleUnusedForDeletion();
+	void markActiveAsUsed(std::optional<Location> _onlyLocation = std::nullopt);
+	void clearActive(std::optional<Location> _onlyLocation = std::nullopt);
 
 	std::optional<YulString> identifierNameIfSSA(Expression const& _expression) const;
 

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

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)
+//     }
+// }

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)
+//     }
+// }

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)
+// }

test/libyul/yulOptimizerTests/unusedAssignEliminator/for_deep_noremove.yul

@@ -46,10 +46,7 @@
 //                         for { } 1 { }
 //                         {
 //                             for { } 1 { a := 10 }
-//                             {
-//                                 b := 12
-//                                 b := 11
-//                             }
+//                             { b := 11 }
 //                         }
 //                     }
 //                 }

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())
+// }