Merge pull request #593 from chriseth/sol_swapConstants

Swap literals to the end if optimizing.

libsolidity/Compiler.cpp

@@ -109,8 +109,8 @@ void Compiler::appendFunctionSelector(ContractDefinition const& _contract)
 		callDataUnpackerEntryPoints.push_back(m_context.newTag());
 		m_context << eth::dupInstruction(2) << eth::dupInstruction(2) << eth::Instruction::EQ;
 		m_context.appendConditionalJumpTo(callDataUnpackerEntryPoints.back());
-		m_context << eth::dupInstruction(4) << eth::Instruction::ADD;
-		//@todo avoid the last ADD (or remove it in the optimizer)
+		if (funid < interfaceFunctions.size() - 1)
+			m_context << eth::dupInstruction(4) << eth::Instruction::ADD;
 	}
 	m_context << eth::Instruction::STOP; // function not found
 
@@ -241,7 +241,7 @@ bool Compiler::visit(FunctionDefinition const& _function)
 
 bool Compiler::visit(IfStatement const& _ifStatement)
 {
-	ExpressionCompiler::compileExpression(m_context, _ifStatement.getCondition());
+	compileExpression(_ifStatement.getCondition());
 	eth::AssemblyItem trueTag = m_context.appendConditionalJump();
 	if (_ifStatement.getFalseStatement())
 		_ifStatement.getFalseStatement()->accept(*this);
@@ -260,7 +260,7 @@ bool Compiler::visit(WhileStatement const& _whileStatement)
 	m_breakTags.push_back(loopEnd);
 
 	m_context << loopStart;
-	ExpressionCompiler::compileExpression(m_context, _whileStatement.getCondition());
+	compileExpression(_whileStatement.getCondition());
 	m_context << eth::Instruction::ISZERO;
 	m_context.appendConditionalJumpTo(loopEnd);
 
@@ -293,7 +293,7 @@ bool Compiler::visit(Return const& _return)
 	//@todo modifications are needed to make this work with functions returning multiple values
 	if (Expression const* expression = _return.getExpression())
 	{
-		ExpressionCompiler::compileExpression(m_context, *expression);
+		compileExpression(*expression);
 		VariableDeclaration const& firstVariable = *_return.getFunctionReturnParameters().getParameters().front();
 		ExpressionCompiler::appendTypeConversion(m_context, *expression->getType(), *firstVariable.getType());
 
@@ -307,7 +307,7 @@ bool Compiler::visit(VariableDefinition const& _variableDefinition)
 {
 	if (Expression const* expression = _variableDefinition.getExpression())
 	{
-		ExpressionCompiler::compileExpression(m_context, *expression);
+		compileExpression(*expression);
 		ExpressionCompiler::appendTypeConversion(m_context,
 												 *expression->getType(),
 												 *_variableDefinition.getDeclaration().getType());
@@ -319,10 +319,15 @@ bool Compiler::visit(VariableDefinition const& _variableDefinition)
 bool Compiler::visit(ExpressionStatement const& _expressionStatement)
 {
 	Expression const& expression = _expressionStatement.getExpression();
-	ExpressionCompiler::compileExpression(m_context, expression);
+	compileExpression(expression);
 	CompilerUtils(m_context).popStackElement(*expression.getType());
 	return false;
 }
 
+void Compiler::compileExpression(Expression const& _expression)
+{
+	ExpressionCompiler::compileExpression(m_context, _expression, m_optimize);
+}
+
 }
 }

libsolidity/Compiler.h

@@ -30,10 +30,10 @@ namespace solidity {
 class Compiler: private ASTConstVisitor
 {
 public:
-	Compiler(): m_returnTag(m_context.newTag()) {}
+	explicit Compiler(bool _optimize = false): m_optimize(_optimize), m_returnTag(m_context.newTag()) {}
 
 	void compileContract(ContractDefinition const& _contract, std::vector<MagicVariableDeclaration const*> const& _magicGlobals);
-	bytes getAssembledBytecode(bool _optimize = false) { return m_context.getAssembledBytecode(_optimize); }
+	bytes getAssembledBytecode() { return m_context.getAssembledBytecode(m_optimize); }
 	void streamAssembly(std::ostream& _stream) const { m_context.streamAssembly(_stream); }
 
 private:
@@ -57,7 +57,9 @@ private:
 	virtual bool visit(VariableDefinition const& _variableDefinition) override;
 	virtual bool visit(ExpressionStatement const& _expressionStatement) override;
 
+	void compileExpression(Expression const& _expression);
 
+	bool const m_optimize;
 	CompilerContext m_context;
 	std::vector<eth::AssemblyItem> m_breakTags; ///< tag to jump to for a "break" statement
 	std::vector<eth::AssemblyItem> m_continueTags; ///< tag to jump to for a "continue" statement

libsolidity/CompilerStack.cpp

@@ -101,10 +101,10 @@ void CompilerStack::compile(bool _optimize)
 			if (ContractDefinition* contract = dynamic_cast<ContractDefinition*>(node.get()))
 			{
 				m_globalContext->setCurrentContract(*contract);
-				shared_ptr<Compiler> compiler = make_shared<Compiler>();
+				shared_ptr<Compiler> compiler = make_shared<Compiler>(_optimize);
 				compiler->compileContract(*contract, m_globalContext->getMagicVariables());
 				Contract& compiledContract = m_contracts[contract->getName()];
-				compiledContract.bytecode = compiler->getAssembledBytecode(_optimize);
+				compiledContract.bytecode = compiler->getAssembledBytecode();
 				compiledContract.compiler = move(compiler);
 			}
 }

libsolidity/ExpressionCompiler.cpp

@@ -33,9 +33,9 @@ using namespace std;
 namespace dev {
 namespace solidity {
 
-void ExpressionCompiler::compileExpression(CompilerContext& _context, Expression const& _expression)
+void ExpressionCompiler::compileExpression(CompilerContext& _context, Expression const& _expression, bool _optimize)
 {
-	ExpressionCompiler compiler(_context);
+	ExpressionCompiler compiler(_context, _optimize);
 	_expression.accept(compiler);
 }
 
@@ -145,10 +145,24 @@ bool ExpressionCompiler::visit(BinaryOperation const& _binaryOperation)
 			if (Token::isCompareOp(op) || op == Token::DIV || op == Token::MOD)
 				cleanupNeeded = true;
 
-		rightExpression.accept(*this);
-		appendTypeConversion(*rightExpression.getType(), commonType, cleanupNeeded);
-		leftExpression.accept(*this);
-		appendTypeConversion(*leftExpression.getType(), commonType, cleanupNeeded);
+		// for commutative operators, push the literal as late as possible to allow improved optimization
+		//@todo this has to be extended for literal expressions
+		bool swap = (m_optimize && Token::isCommutativeOp(op) && dynamic_cast<Literal const*>(&rightExpression)
+					 && !dynamic_cast<Literal const*>(&leftExpression));
+		if (swap)
+		{
+			leftExpression.accept(*this);
+			appendTypeConversion(*leftExpression.getType(), commonType, cleanupNeeded);
+			rightExpression.accept(*this);
+			appendTypeConversion(*rightExpression.getType(), commonType, cleanupNeeded);
+		}
+		else
+		{
+			rightExpression.accept(*this);
+			appendTypeConversion(*rightExpression.getType(), commonType, cleanupNeeded);
+			leftExpression.accept(*this);
+			appendTypeConversion(*leftExpression.getType(), commonType, cleanupNeeded);
+		}
 		if (Token::isCompareOp(op))
 			appendCompareOperatorCode(op, commonType);
 		else

libsolidity/ExpressionCompiler.h

@@ -46,14 +46,14 @@ class ExpressionCompiler: private ASTConstVisitor
 {
 public:
 	/// Compile the given @a _expression into the @a _context.
-	static void compileExpression(CompilerContext& _context, Expression const& _expression);
+	static void compileExpression(CompilerContext& _context, Expression const& _expression, bool _optimize = false);
 
 	/// Appends code to remove dirty higher order bits in case of an implicit promotion to a wider type.
 	static void appendTypeConversion(CompilerContext& _context, Type const& _typeOnStack, Type const& _targetType);
 
 private:
-	ExpressionCompiler(CompilerContext& _compilerContext):
-		m_context(_compilerContext), m_currentLValue(m_context) {}
+	explicit ExpressionCompiler(CompilerContext& _compilerContext, bool _optimize = false):
+		m_optimize(_optimize), m_context(_compilerContext), m_currentLValue(m_context) {}
 
 	virtual bool visit(Assignment const& _assignment) override;
 	virtual void endVisit(UnaryOperation const& _unaryOperation) override;
@@ -133,6 +133,7 @@ private:
 		unsigned m_stackSize;
 	};
 
+	bool m_optimize;
 	CompilerContext& m_context;
 	LValue m_currentLValue;
 };

libsolidity/Token.h

@@ -350,6 +350,8 @@ public:
 	static bool isElementaryTypeName(Value tok) { return INT <= tok && tok < TYPES_END; }
 	static bool isAssignmentOp(Value tok) { return ASSIGN <= tok && tok <= ASSIGN_MOD; }
 	static bool isBinaryOp(Value op) { return COMMA <= op && op <= MOD; }
+	static bool isCommutativeOp(Value op) { return op == BIT_OR || op == BIT_XOR || op == BIT_AND ||
+				op == ADD || op == MUL || op == EQ || op == NE; }
 	static bool isArithmeticOp(Value op) { return ADD <= op && op <= MOD; }
 	static bool isCompareOp(Value op) { return EQ <= op && op <= IN; }
 

test/solidityCompiler.cpp

@@ -87,7 +87,7 @@ BOOST_AUTO_TEST_CASE(smoke_test)
 							 "}\n";
 	bytes code = compileContract(sourceCode);
 
-	unsigned boilerplateSize = 42;
+	unsigned boilerplateSize = 40;
 	bytes expectation({byte(Instruction::JUMPDEST),
 					   byte(Instruction::PUSH1), 0x0, // initialize local variable x
 					   byte(Instruction::PUSH1), 0x2,
@@ -107,8 +107,8 @@ BOOST_AUTO_TEST_CASE(different_argument_numbers)
 							 "}\n";
 	bytes code = compileContract(sourceCode);
 
-	unsigned shift = 70;
-	unsigned boilerplateSize = 83;
+	unsigned shift = 68;
+	unsigned boilerplateSize = 81;
 	bytes expectation({byte(Instruction::JUMPDEST),
 					   byte(Instruction::PUSH1), 0x0, // initialize return variable d
 					   byte(Instruction::DUP3),
@@ -158,8 +158,8 @@ BOOST_AUTO_TEST_CASE(ifStatement)
 							 "}\n";
 	bytes code = compileContract(sourceCode);
 
-	unsigned shift = 29;
-	unsigned boilerplateSize = 42;
+	unsigned shift = 27;
+	unsigned boilerplateSize = 40;
 	bytes expectation({byte(Instruction::JUMPDEST),
 					   byte(Instruction::PUSH1), 0x0,
 					   byte(Instruction::DUP1),
@@ -200,8 +200,8 @@ BOOST_AUTO_TEST_CASE(loops)
 							 "}\n";
 	bytes code = compileContract(sourceCode);
 
-	unsigned shift = 29;
-	unsigned boilerplateSize = 42;
+	unsigned shift = 27;
+	unsigned boilerplateSize = 40;
 	bytes expectation({byte(Instruction::JUMPDEST),
 					   byte(Instruction::JUMPDEST),
 					   byte(Instruction::PUSH1), 0x1,

test/solidityOptimizerTest.cpp

@@ -48,7 +48,7 @@ public:
 		m_optimize = true;
 		bytes optimizedBytecode = compileAndRun(_sourceCode, _value, _contractName);
 		int sizeDiff = nonOptimizedBytecode.size() - optimizedBytecode.size();
-		BOOST_CHECK_MESSAGE(sizeDiff >= (int)_expectedSizeDecrease, "Bytecode did only shrink by "
+		BOOST_CHECK_MESSAGE(sizeDiff == int(_expectedSizeDecrease), "Bytecode did only shrink by "
 							+ boost::lexical_cast<string>(sizeDiff) + " bytes, expected: "
 							+ boost::lexical_cast<string>(_expectedSizeDecrease));
 		m_optimizedContract = m_contractAddress;
@@ -106,7 +106,7 @@ BOOST_AUTO_TEST_CASE(invariants)
 				return (((a + (1 - 1)) ^ 0) | 0) & (uint(0) - 1);
 			}
 		})";
-	compileBothVersions(19, sourceCode);
+	compileBothVersions(28, sourceCode);
 	compareVersions(0, u256(0x12334664));
 }
 
@@ -124,6 +124,21 @@ BOOST_AUTO_TEST_CASE(unused_expressions)
 	compareVersions(0);
 }
 
+BOOST_AUTO_TEST_CASE(constant_folding_both_sides)
+{
+	// if constants involving the same associative and commutative operator are applied from both
+	// sides, the operator should be applied only once, because the expression compiler
+	// (even in non-optimized mode) pushes literals as late as possible
+	char const* sourceCode = R"(
+		contract test {
+			function f(uint x) returns (uint y) {
+				return 98 ^ (7 * ((1 | (x | 1000)) * 40) ^ 102);
+			}
+		})";
+	compileBothVersions(31, sourceCode);
+	compareVersions(0);
+}
+
 BOOST_AUTO_TEST_SUITE_END()
 
 }