Merge pull request #13752 from ethereum/reimplement_unused_assign

Reimplement unused eliminators

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);
+
+	uae.m_storesToRemove += uae.m_allStores - uae.m_usedStores;
 
-	StatementRemover remover{rae.m_pendingRemovals};
+	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,84 +75,87 @@ 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)
 {
 	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);
-}
-
-void UnusedAssignEliminator::changeUndecidedTo(YulString _variable, UnusedAssignEliminator::State _newState)
-{
-	for (auto& assignment: m_stores[_variable])
-		if (assignment.second == State::Undecided)
-			assignment.second = _newState;
+		markUsed(retParam.name);
 }
 
-void UnusedAssignEliminator::finalize(YulString _variable, UnusedAssignEliminator::State _finalState)
+void UnusedAssignEliminator::markUsed(YulString _variable)
 {
-	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".
- *
- * 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".
+ * At ``leave`` statements, all return variables are set to "used" and the set of active statements
+ * is cleared.
  *
- * 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,31 +119,32 @@ class UnusedAssignEliminator: public UnusedStoreBase
 	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;
 
 	using UnusedStoreBase::visit;
 	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;
 };
 
 }