Merge pull request #6276 from ethereum/properlyDetectRemovableVariables

Properly detect removable variables

Changelog.md

@@ -11,6 +11,7 @@ Compiler Features:
 
 Bugfixes:
  * Code Generator: Defensively pad memory for ``type(Contract).name`` to multiples of 32.
+ * Yul Optimizer: Properly determine whether a variable can be eliminated during stack compression pass.
 
 
 Build System:

libsolidity/codegen/CompilerContext.cpp

@@ -428,6 +428,7 @@ void CompilerContext::appendInlineAssembly(
 			yul::EVMDialect::strictAssemblyForEVM(m_evmVersion),
 			*parserResult,
 			analysisInfo,
+			_optimiserSettings.optimizeStackAllocation,
 			externallyUsedIdentifiers
 		);
 		analysisInfo = yul::AsmAnalysisInfo{};

libyul/AssemblyStack.cpp

@@ -135,7 +135,10 @@ void AssemblyStack::optimize(Object& _object)
 	for (auto& subNode: _object.subObjects)
 		if (auto subObject = dynamic_cast<Object*>(subNode.get()))
 			optimize(*subObject);
-	OptimiserSuite::run(languageToDialect(m_language, m_evmVersion), *_object.code, *_object.analysisInfo);
+	// TODO: Store this as setting - it should be the same as the flag passed to
+	// ::assemble(...)
+	bool optimizeStackAllocation = false;
+	OptimiserSuite::run(languageToDialect(m_language, m_evmVersion), *_object.code, *_object.analysisInfo, optimizeStackAllocation);
 }
 
 MachineAssemblyObject AssemblyStack::assemble(Machine _machine, bool _optimize) const

libyul/CompilabilityChecker.cpp

@@ -33,7 +33,11 @@ using namespace yul;
 using namespace dev;
 using namespace dev::solidity;
 
-std::map<YulString, int> CompilabilityChecker::run(std::shared_ptr<Dialect> _dialect, Block const& _ast)
+map<YulString, int> CompilabilityChecker::run(
+	shared_ptr<Dialect> _dialect,
+	Block const& _ast,
+	bool _optimizeStackAllocation
+)
 {
 	if (_dialect->flavour == AsmFlavour::Yul)
 		return {};
@@ -43,12 +47,11 @@ std::map<YulString, int> CompilabilityChecker::run(std::shared_ptr<Dialect> _dia
 	solAssert(dynamic_cast<EVMDialect const*>(_dialect.get()), "");
 	shared_ptr<NoOutputEVMDialect> noOutputDialect = make_shared<NoOutputEVMDialect>(dynamic_pointer_cast<EVMDialect>(_dialect));
 
-	bool optimize = true;
 	yul::AsmAnalysisInfo analysisInfo =
 		yul::AsmAnalyzer::analyzeStrictAssertCorrect(noOutputDialect, _ast);
 
 	NoOutputAssembly assembly;
-	CodeTransform transform(assembly, analysisInfo, _ast, *noOutputDialect, optimize);
+	CodeTransform transform(assembly, analysisInfo, _ast, *noOutputDialect, _optimizeStackAllocation);
 	try
 	{
 		transform(_ast);

libyul/CompilabilityChecker.h

@@ -39,7 +39,11 @@ namespace yul
 class CompilabilityChecker
 {
 public:
-	static std::map<YulString, int> run(std::shared_ptr<Dialect> _dialect, Block const& _ast);
+	static std::map<YulString, int> run(
+		std::shared_ptr<Dialect> _dialect,
+		Block const& _ast,
+		bool _optimizeStackAllocation
+	);
 };
 
 }

libyul/optimiser/StackCompressor.cpp

@@ -39,29 +39,87 @@ using namespace yul;
 namespace
 {
 
+/**
+ * Class that discovers all variables that can be fully eliminated by rematerialization,
+ * and the corresponding approximate costs.
+ */
+class RematCandidateSelector: public DataFlowAnalyzer
+{
+public:
+	explicit RematCandidateSelector(Dialect const& _dialect): DataFlowAnalyzer(_dialect) {}
+
+	/// @returns a set of pairs of rematerialisation costs and variable to rematerialise.
+	/// Note that this set is sorted by cost.
+	set<pair<size_t, YulString>> candidates()
+	{
+		set<pair<size_t, YulString>> cand;
+		for (auto const& codeCost: m_expressionCodeCost)
+		{
+			size_t numRef = m_numReferences[codeCost.first];
+			cand.emplace(make_pair(codeCost.second * numRef, codeCost.first));
+		}
+		return cand;
+	}
+
+	using DataFlowAnalyzer::operator();
+	void operator()(VariableDeclaration& _varDecl) override
+	{
+		DataFlowAnalyzer::operator()(_varDecl);
+		if (_varDecl.variables.size() == 1)
+		{
+			YulString varName = _varDecl.variables.front().name;
+			if (m_value.count(varName))
+				m_expressionCodeCost[varName] = CodeCost::codeCost(*m_value[varName]);
+		}
+	}
+
+	void operator()(Assignment& _assignment) override
+	{
+		for (auto const& var: _assignment.variableNames)
+			rematImpossible(var.name);
+		DataFlowAnalyzer::operator()(_assignment);
+	}
+
+	// We use visit(Expression) because operator()(Identifier) would also
+	// get called on left-hand-sides of assignments.
+	void visit(Expression& _e) override
+	{
+		if (_e.type() == typeid(Identifier))
+		{
+			YulString name = boost::get<Identifier>(_e).name;
+			if (m_expressionCodeCost.count(name))
+			{
+				if (!m_value.count(name))
+					rematImpossible(name);
+				else
+					++m_numReferences[name];
+			}
+		}
+		DataFlowAnalyzer::visit(_e);
+	}
+
+	/// Remove the variable from the candidate set.
+	void rematImpossible(YulString _variable)
+	{
+		m_numReferences.erase(_variable);
+		m_expressionCodeCost.erase(_variable);
+	}
+
+	/// Candidate variables and the code cost of their value.
+	map<YulString, size_t> m_expressionCodeCost;
+	/// Number of references to each candidate variable.
+	map<YulString, size_t> m_numReferences;
+};
+
 template <typename ASTNode>
 void eliminateVariables(shared_ptr<Dialect> const& _dialect, ASTNode& _node, size_t _numVariables)
 {
-	SSAValueTracker ssaValues;
-	ssaValues(_node);
-
-	map<YulString, size_t> references = ReferencesCounter::countReferences(_node);
-
-	set<pair<size_t, YulString>> rematCosts;
-	for (auto const& ssa: ssaValues.values())
-	{
-		if (!MovableChecker{*_dialect, *ssa.second}.movable())
-			continue;
-		size_t numRef = references[ssa.first];
-		size_t cost = 0;
-		if (numRef > 1)
-			cost = CodeCost::codeCost(*ssa.second) * (numRef - 1);
-		rematCosts.insert(make_pair(cost, ssa.first));
-	}
+	RematCandidateSelector selector{*_dialect};
+	selector(_node);
 
 	// Select at most _numVariables
 	set<YulString> varsToEliminate;
-	for (auto const& costs: rematCosts)
+	for (auto const& costs: selector.candidates())
 	{
 		if (varsToEliminate.size() >= _numVariables)
 			break;
@@ -74,15 +132,20 @@ void eliminateVariables(shared_ptr<Dialect> const& _dialect, ASTNode& _node, siz
 
 }
 
-bool StackCompressor::run(shared_ptr<Dialect> const& _dialect, Block& _ast)
+bool StackCompressor::run(
+	shared_ptr<Dialect> const& _dialect,
+	Block& _ast,
+	bool _optimizeStackAllocation,
+	size_t _maxIterations
+)
 {
 	yulAssert(
 		_ast.statements.size() > 0 && _ast.statements.at(0).type() == typeid(Block),
 		"Need to run the function grouper before the stack compressor."
 	);
-	for (size_t iterations = 0; iterations < 4; iterations++)
+	for (size_t iterations = 0; iterations < _maxIterations; iterations++)
 	{
-		map<YulString, int> stackSurplus = CompilabilityChecker::run(_dialect, _ast);
+		map<YulString, int> stackSurplus = CompilabilityChecker::run(_dialect, _ast, _optimizeStackAllocation);
 		if (stackSurplus.empty())
 			return true;
 

libyul/optimiser/StackCompressor.h

@@ -41,7 +41,12 @@ class StackCompressor
 public:
 	/// Try to remove local variables until the AST is compilable.
 	/// @returns true if it was successful.
-	static bool run(std::shared_ptr<Dialect> const& _dialect, Block& _ast);
+	static bool run(
+		std::shared_ptr<Dialect> const& _dialect,
+		Block& _ast,
+		bool _optimizeStackAllocation,
+		size_t _maxIterations
+	);
 };
 
 }

libyul/optimiser/Suite.cpp

@@ -59,6 +59,7 @@ void OptimiserSuite::run(
 	shared_ptr<Dialect> const& _dialect,
 	Block& _ast,
 	AsmAnalysisInfo const& _analysisInfo,
+	bool _optimizeStackAllocation,
 	set<YulString> const& _externallyUsedIdentifiers
 )
 {
@@ -184,8 +185,12 @@ void OptimiserSuite::run(
 	Rematerialiser::run(*_dialect, ast);
 	UnusedPruner::runUntilStabilised(*_dialect, ast, reservedIdentifiers);
 
+	// This is a tuning parameter, but actually just prevents infinite loops.
+	size_t stackCompressorMaxIterations = 16;
 	FunctionGrouper{}(ast);
-	StackCompressor::run(_dialect, ast);
+	// We ignore the return value because we will get a much better error
+	// message once we perform code generation.
+	StackCompressor::run(_dialect, ast, _optimizeStackAllocation, stackCompressorMaxIterations);
 	BlockFlattener{}(ast);
 
 	VarNameCleaner{ast, *_dialect, reservedIdentifiers}(ast);

libyul/optimiser/Suite.h

@@ -42,6 +42,7 @@ public:
 		std::shared_ptr<Dialect> const& _dialect,
 		Block& _ast,
 		AsmAnalysisInfo const& _analysisInfo,
+		bool _optimizeStackAllocation,
 		std::set<YulString> const& _externallyUsedIdentifiers = {}
 	);
 };

test/libyul/CompilabilityChecker.cpp

@@ -39,7 +39,7 @@ string check(string const& _input)
 {
 	shared_ptr<Block> ast = yul::test::parse(_input, false).first;
 	BOOST_REQUIRE(ast);
-	map<YulString, int> functions = CompilabilityChecker::run(EVMDialect::strictAssemblyForEVM(dev::test::Options::get().evmVersion()), *ast);
+	map<YulString, int> functions = CompilabilityChecker::run(EVMDialect::strictAssemblyForEVM(dev::test::Options::get().evmVersion()), *ast, true);
 	string out;
 	for (auto const& function: functions)
 		out += function.first.str() + ": " + to_string(function.second) + " ";

test/libyul/YulOptimizerTest.cpp

@@ -247,11 +247,12 @@ bool YulOptimizerTest::run(ostream& _stream, string const& _linePrefix, bool con
 	{
 		disambiguate();
 		(FunctionGrouper{})(*m_ast);
-		StackCompressor::run(m_dialect, *m_ast);
+		size_t maxIterations = 16;
+		StackCompressor::run(m_dialect, *m_ast, true, maxIterations);
 		(BlockFlattener{})(*m_ast);
 	}
 	else if (m_optimizerStep == "fullSuite")
-		OptimiserSuite::run(m_dialect, *m_ast, *m_analysisInfo);
+		OptimiserSuite::run(m_dialect, *m_ast, *m_analysisInfo, true);
 	else
 	{
 		AnsiColorized(_stream, _formatted, {formatting::BOLD, formatting::RED}) << _linePrefix << "Invalid optimizer step: " << m_optimizerStep << endl;

test/libyul/yulOptimizerTests/fullSuite/aztec.yul

@@ -231,11 +231,11 @@
 // ----
 // fullSuite
 // {
-//     let _1 := 0x80
-//     mstore(_1, 7673901602397024137095011250362199966051872585513276903826533215767972925880)
+//     mstore(0x80, 7673901602397024137095011250362199966051872585513276903826533215767972925880)
 //     mstore(0xa0, 8489654445897228341090914135473290831551238522473825886865492707826370766375)
+//     let notes := add(0x04, calldataload(0x04))
 //     let m := calldataload(0x24)
-//     let n := calldataload(add(0x04, calldataload(0x04)))
+//     let n := calldataload(notes)
 //     let gen_order := 0x30644e72e131a029b85045b68181585d2833e84879b9709143e1f593f0000001
 //     let challenge := mod(calldataload(0x44), gen_order)
 //     if gt(m, n)
@@ -248,8 +248,8 @@
 //     mstore(0x2c0, kn)
 //     mstore(0x2e0, m)
 //     kn := mulmod(sub(gen_order, kn), challenge, gen_order)
-//     hashCommitments(add(0x04, calldataload(0x04)), n)
-//     let b := add(0x300, mul(n, _1))
+//     hashCommitments(notes, n)
+//     let b := add(0x300, mul(n, 0x80))
 //     let i := 0
 //     let i_1 := i
 //     for {
@@ -259,11 +259,13 @@
 //         i := add(i, 0x01)
 //     }
 //     {
-//         let _2 := add(calldataload(0x04), mul(i, 0xc0))
+//         let _1 := add(calldataload(0x04), mul(i, 0xc0))
+//         let noteIndex := add(_1, 0x24)
 //         let k := i_1
-//         let a := calldataload(add(_2, 0x44))
+//         let a := calldataload(add(_1, 0x44))
 //         let c := challenge
-//         switch eq(add(i, 0x01), n)
+//         let _2 := add(i, 0x01)
+//         switch eq(_2, n)
 //         case 1 {
 //             k := kn
 //             if eq(m, n)
@@ -272,10 +274,10 @@
 //             }
 //         }
 //         case 0 {
-//             k := calldataload(add(_2, 0x24))
+//             k := calldataload(noteIndex)
 //         }
-//         validateCommitment(add(_2, 0x24), k, a)
-//         switch gt(add(i, 0x01), m)
+//         validateCommitment(noteIndex, k, a)
+//         switch gt(_2, m)
 //         case 1 {
 //             kn := addmod(kn, sub(gen_order, k), gen_order)
 //             let x := mod(mload(i_1), gen_order)
@@ -288,17 +290,16 @@
 //             kn := addmod(kn, k, gen_order)
 //         }
 //         let _3 := 0x40
-//         calldatacopy(0xe0, add(_2, 164), _3)
-//         calldatacopy(0x20, add(_2, 100), _3)
+//         calldatacopy(0xe0, add(_1, 164), _3)
+//         calldatacopy(0x20, add(_1, 100), _3)
 //         mstore(0x120, sub(gen_order, c))
-//         let _4 := 0x60
-//         mstore(_4, k)
+//         mstore(0x60, k)
 //         mstore(0xc0, a)
-//         let result := call(gas(), 7, i_1, 0xe0, _4, 0x1a0, _3)
-//         let result_1 := and(result, call(gas(), 7, i_1, 0x20, _4, 0x120, _3))
-//         let result_2 := and(result_1, call(gas(), 7, i_1, _1, _4, 0x160, _3))
-//         let result_3 := and(result_2, call(gas(), 6, i_1, 0x120, _1, 0x160, _3))
-//         result := and(result_3, call(gas(), 6, i_1, 0x160, _1, b, _3))
+//         let result := call(gas(), 7, i_1, 0xe0, 0x60, 0x1a0, _3)
+//         let result_1 := and(result, call(gas(), 7, i_1, 0x20, 0x60, 0x120, _3))
+//         let result_2 := and(result_1, call(gas(), 7, i_1, 0x80, 0x60, 0x160, _3))
+//         let result_3 := and(result_2, call(gas(), 6, i_1, 0x120, 0x80, 0x160, _3))
+//         result := and(result_3, call(gas(), 6, i_1, 0x160, 0x80, b, _3))
 //         if eq(i, m)
 //         {
 //             mstore(0x260, mload(0x20))
@@ -308,10 +309,10 @@
 //         }
 //         if gt(i, m)
 //         {
-//             mstore(_4, c)
-//             let result_4 := and(result, call(gas(), 7, i_1, 0x20, _4, 0x220, _3))
-//             let result_5 := and(result_4, call(gas(), 6, i_1, 0x220, _1, 0x260, _3))
-//             result := and(result_5, call(gas(), 6, i_1, 0x1a0, _1, 0x1e0, _3))
+//             mstore(0x60, c)
+//             let result_4 := and(result, call(gas(), 7, i_1, 0x20, 0x60, 0x220, _3))
+//             let result_5 := and(result_4, call(gas(), 6, i_1, 0x220, 0x80, 0x260, _3))
+//             result := and(result_5, call(gas(), 6, i_1, 0x1a0, 0x80, 0x1e0, _3))
 //         }
 //         if iszero(result)
 //         {

test/tools/yulopti.cpp

@@ -198,7 +198,7 @@ public:
 				SSAReverser::run(*m_ast);
 				break;
 			case 'p':
-				StackCompressor::run(m_dialect, *m_ast);
+				StackCompressor::run(m_dialect, *m_ast, true, 16);
 				break;
 			default:
 				cout << "Unknown option." << endl;