Merge pull request #5269 from ethereum/cseBugfix

Bugfix in common subexpression eliminator related to scopes.

libyul/optimiser/CommonSubexpressionEliminator.cpp

@@ -50,7 +50,7 @@ void CommonSubexpressionEliminator::visit(Expression& _e)
 			if (m_value.at(name)->type() == typeid(Identifier))
 			{
 				string const& value = boost::get<Identifier>(*m_value.at(name)).name;
-				if (inScope(value))
+				assertThrow(inScope(value), OptimizerException, "");
 				_e = Identifier{locationOf(_e), value};
 			}
 		}
@@ -61,6 +61,7 @@ void CommonSubexpressionEliminator::visit(Expression& _e)
 		for (auto const& var: m_value)
 		{
 			assertThrow(var.second, OptimizerException, "");
+			assertThrow(inScope(var.first), OptimizerException, "");
 			if (SyntacticalEqualityChecker::equal(_e, *var.second))
 			{
 				_e = Identifier{locationOf(_e), var.first};

libyul/optimiser/DataFlowAnalyzer.cpp

@@ -84,19 +84,19 @@ void DataFlowAnalyzer::operator()(Switch& _switch)
 
 void DataFlowAnalyzer::operator()(FunctionDefinition& _fun)
 {
-	m_variableScopes.emplace_back(true);
+	pushScope(true);
 	for (auto const& parameter: _fun.parameters)
 		m_variableScopes.back().variables.insert(parameter.name);
 	for (auto const& var: _fun.returnVariables)
 		m_variableScopes.back().variables.insert(var.name);
 	ASTModifier::operator()(_fun);
-	m_variableScopes.pop_back();
+	popScope();
 }
 
 void DataFlowAnalyzer::operator()(ForLoop& _for)
 {
 	// Special scope handling of the pre block.
-	m_variableScopes.emplace_back(false);
+	pushScope(false);
 	for (auto& statement: _for.pre.statements)
 		visit(statement);
 
@@ -110,16 +110,15 @@ void DataFlowAnalyzer::operator()(ForLoop& _for)
 	(*this)(_for.post);
 
 	clearValues(assignments.names());
-
+	popScope();
-	m_variableScopes.pop_back();
 }
 
 void DataFlowAnalyzer::operator()(Block& _block)
 {
 	size_t numScopes = m_variableScopes.size();
-	m_variableScopes.emplace_back(false);
+	pushScope(false);
 	ASTModifier::operator()(_block);
-	m_variableScopes.pop_back();
+	popScope();
 	assertThrow(numScopes == m_variableScopes.size(), OptimizerException, "");
 }
 
@@ -148,7 +147,18 @@ void DataFlowAnalyzer::handleAssignment(set<string> const& _variables, Expressio
 	}
 }
 
-void DataFlowAnalyzer::clearValues(set<string> const& _variables)
+void DataFlowAnalyzer::pushScope(bool _functionScope)
+{
+	m_variableScopes.emplace_back(_functionScope);
+}
+
+void DataFlowAnalyzer::popScope()
+{
+	clearValues(std::move(m_variableScopes.back().variables));
+	m_variableScopes.pop_back();
+}
+
+void DataFlowAnalyzer::clearValues(set<string> _variables)
 {
 	// All variables that reference variables to be cleared also have to be
 	// cleared, but not recursively, since only the value of the original
@@ -163,16 +173,15 @@ void DataFlowAnalyzer::clearValues(set<string> const& _variables)
 	// This cannot be easily tested since the substitutions will be done
 	// one by one on the fly, and the last line will just be add(1, 1)
 
-	set<string> variables = _variables;
 	// Clear variables that reference variables to be cleared.
-	for (auto const& name: variables)
+	for (auto const& name: _variables)
 		for (auto const& ref: m_referencedBy[name])
-			variables.insert(ref);
+			_variables.insert(ref);
 
 	// Clear the value and update the reference relation.
-	for (auto const& name: variables)
+	for (auto const& name: _variables)
 		m_value.erase(name);
-	for (auto const& name: variables)
+	for (auto const& name: _variables)
 	{
 		for (auto const& ref: m_references[name])
 			m_referencedBy[ref].erase(name);

libyul/optimiser/DataFlowAnalyzer.h

@@ -56,9 +56,15 @@ protected:
 	/// Registers the assignment.
 	void handleAssignment(std::set<std::string> const& _names, Expression* _value);
 
+	/// Creates a new inner scope.
+	void pushScope(bool _functionScope);
+
+	/// Removes the innermost scope and clears all variables in it.
+	void popScope();
+
 	/// Clears information about the values assigned to the given variables,
 	/// for example at points where control flow is merged.
-	void clearValues(std::set<std::string> const& _names);
+	void clearValues(std::set<std::string> _names);
 
 	/// Returns true iff the variable is in scope.
 	bool inScope(std::string const& _variableName) const;

libyul/optimiser/Rematerialiser.cpp

@@ -38,16 +38,11 @@ void Rematerialiser::visit(Expression& _e)
 		if (m_value.count(identifier.name))
 		{
 			string name = identifier.name;
-			bool expressionValid = true;
 			for (auto const& ref: m_references[name])
-				if (!inScope(ref))
+				assertThrow(inScope(ref), OptimizerException, "");
-				{
-					expressionValid = false;
-					break;
-				}
 			assertThrow(m_value.at(name), OptimizerException, "");
 			auto const& value = *m_value.at(name);
-			if (expressionValid && CodeSize::codeSize(value) <= 7)
+			if (CodeSize::codeSize(value) <= 7)
 				_e = (ASTCopier{}).translate(value);
 		}
 	}

test/libyul/yulOptimizerTests/commonSubexpressionEliminator/scopes.yul

@@ -0,0 +1,25 @@
+{
+    let a := 10
+    let x := 20
+    {
+        let b := calldataload(0)
+        let d := calldataload(1)
+        x := d
+    }
+    // We had a bug where "calldataload(0)" was incorrectly replaced by "b"
+    mstore(0, calldataload(0))
+    mstore(0, x)
+}
+// ----
+// commonSubexpressionEliminator
+// {
+//     let a := 10
+//     let x := 20
+//     {
+//         let b := calldataload(0)
+//         let d := calldataload(1)
+//         x := d
+//     }
+//     mstore(0, calldataload(0))
+//     mstore(0, x)
+// }