Merge pull request #2027 from chriseth/sol_dupRemForLoops

Allow duplicate code removal for loops.

libevmasm/Assembly.cpp

@@ -307,6 +307,11 @@ Assembly& Assembly::optimise(bool _enable)
 		count = 0;
 
 		copt << "Performing optimisation...";
+		// This only modifies PushTags, we have to run again to actually remove code.
+		BlockDeduplicator dedup(m_items);
+		if (dedup.deduplicate())
+			count++;
+
 		{
 			ControlFlowGraph cfg(m_items);
 			AssemblyItems optimisedItems;
@@ -349,11 +354,6 @@ Assembly& Assembly::optimise(bool _enable)
 				m_items = move(optimisedItems);
 				count++;
 			}
-
-			// This only modifies PushTags, we have to run again to actually remove code.
-			BlockDeduplicator dedup(m_items);
-			if (dedup.deduplicate())
-				count++;
 		}
 	}
 

libevmasm/BlockDeduplicator.cpp

@@ -35,13 +35,33 @@ bool BlockDeduplicator::deduplicate()
 {
 	// Compares indices based on the suffix that starts there, ignoring tags and stopping at
 	// opcodes that stop the control flow.
+
+	// Virtual tag that signifies "the current block" and which is used to optimise loops.
+	// We abort if this virtual tag actually exists.
+	AssemblyItem pushSelf(PushTag, u256(-4));
+	if (
+		std::count(m_items.cbegin(), m_items.cend(), pushSelf.tag()) ||
+		std::count(m_items.cbegin(), m_items.cend(), pushSelf.pushTag())
+	)
+		return false;
+
 	function<bool(size_t, size_t)> comparator = [&](size_t _i, size_t _j)
 	{
 		if (_i == _j)
 			return false;
 
-		BlockIterator first(m_items.begin() + _i, m_items.end());
-		BlockIterator second(m_items.begin() + _j, m_items.end());
+		// To compare recursive loops, we have to already unify PushTag opcodes of the
+		// block's own tag.
+		AssemblyItem pushFirstTag(pushSelf);
+		AssemblyItem pushSecondTag(pushSelf);
+
+		if (_i < m_items.size() && m_items.at(_i).type() == Tag)
+			pushFirstTag = m_items.at(_i).pushTag();
+		if (_j < m_items.size() && m_items.at(_j).type() == Tag)
+			pushSecondTag = m_items.at(_j).pushTag();
+
+		BlockIterator first(m_items.begin() + _i, m_items.end(), &pushFirstTag, &pushSelf);
+		BlockIterator second(m_items.begin() + _j, m_items.end(), &pushSecondTag, &pushSelf);
 		BlockIterator end(m_items.end(), m_items.end());
 
 		if (first != end && (*first).type() == Tag)
@@ -52,27 +72,34 @@ bool BlockDeduplicator::deduplicate()
 		return std::lexicographical_compare(first, end, second, end);
 	};
 
-	set<size_t, function<bool(size_t, size_t)>> blocksSeen(comparator);
-	map<u256, u256> tagReplacement;
-	for (size_t i = 0; i < m_items.size(); ++i)
+	size_t iterations = 0;
+	for (; ; ++iterations)
 	{
-		if (m_items.at(i).type() != Tag)
-			continue;
-		auto it = blocksSeen.find(i);
-		if (it == blocksSeen.end())
-			blocksSeen.insert(i);
-		else
-			tagReplacement[m_items.at(i).data()] = m_items.at(*it).data();
-	}
-
-	bool ret = false;
-	for (AssemblyItem& item: m_items)
-		if (item.type() == PushTag && tagReplacement.count(item.data()))
+		//@todo this should probably be optimized.
+		set<size_t, function<bool(size_t, size_t)>> blocksSeen(comparator);
+		map<u256, u256> tagReplacement;
+		for (size_t i = 0; i < m_items.size(); ++i)
 		{
-			ret = true;
-			item.setData(tagReplacement.at(item.data()));
+			if (m_items.at(i).type() != Tag)
+				continue;
+			auto it = blocksSeen.find(i);
+			if (it == blocksSeen.end())
+				blocksSeen.insert(i);
+			else
+				tagReplacement[m_items.at(i).data()] = m_items.at(*it).data();
 		}
-	return ret;
+
+		bool changed = false;
+		for (AssemblyItem& item: m_items)
+			if (item.type() == PushTag && tagReplacement.count(item.data()))
+			{
+				changed = true;
+				item.setData(tagReplacement.at(item.data()));
+			}
+		if (!changed)
+			break;
+	}
+	return iterations > 0;
 }
 
 BlockDeduplicator::BlockIterator& BlockDeduplicator::BlockIterator::operator++()
@@ -89,3 +116,11 @@ BlockDeduplicator::BlockIterator& BlockDeduplicator::BlockIterator::operator++()
 	}
 	return *this;
 }
+
+AssemblyItem const& BlockDeduplicator::BlockIterator::operator*() const
+{
+	if (replaceItem && replaceWith && *it == *replaceItem)
+		return *replaceWith;
+	else
+		return *it;
+}

libevmasm/BlockDeduplicator.h

@@ -47,19 +47,27 @@ public:
 	bool deduplicate();
 
 private:
-	/// Iterator that skips tags skips to the end if (all branches of) the control
+	/// Iterator that skips tags and skips to the end if (all branches of) the control
 	/// flow does not continue to the next instruction.
+	/// If the arguments are supplied to the constructor, replaces items on the fly.
 	struct BlockIterator: std::iterator<std::forward_iterator_tag, AssemblyItem const>
 	{
 	public:
-		BlockIterator(AssemblyItems::const_iterator _it, AssemblyItems::const_iterator _end):
-			it(_it), end(_end) { }
+		BlockIterator(
+			AssemblyItems::const_iterator _it,
+			AssemblyItems::const_iterator _end,
+			AssemblyItem const* _replaceItem = nullptr,
+			AssemblyItem const* _replaceWith = nullptr
+		):
+			it(_it), end(_end), replaceItem(_replaceItem), replaceWith(_replaceWith) {}
 		BlockIterator& operator++();
 		bool operator==(BlockIterator const& _other) const { return it == _other.it; }
 		bool operator!=(BlockIterator const& _other) const { return it != _other.it; }
-		AssemblyItem const& operator*() const { return *it; }
+		AssemblyItem const& operator*() const;
 		AssemblyItems::const_iterator it;
 		AssemblyItems::const_iterator end;
+		AssemblyItem const* replaceItem;
+		AssemblyItem const* replaceWith;
 	};
 
 	AssemblyItems& m_items;

libsolidity/ArrayUtils.cpp

@@ -364,7 +364,13 @@ void ArrayUtils::clearStorageLoop(Type const& _type) const
 		return;
 	}
 	// stack: end_pos pos
-	eth::AssemblyItem loopStart = m_context.newTag();
+
+	// jump to and return from the loop to allow for duplicate code removal
+	eth::AssemblyItem returnTag = m_context.pushNewTag();
+	m_context << eth::Instruction::SWAP2 << eth::Instruction::SWAP1;
+
+	// stack: <return tag> end_pos pos
+	eth::AssemblyItem loopStart = m_context.appendJumpToNew();
 	m_context << loopStart;
 	// check for loop condition
 	m_context << eth::Instruction::DUP1 << eth::Instruction::DUP3
@@ -380,7 +386,11 @@ void ArrayUtils::clearStorageLoop(Type const& _type) const
 	m_context.appendJumpTo(loopStart);
 	// cleanup
 	m_context << zeroLoopEnd;
-	m_context << eth::Instruction::POP;
+	m_context << eth::Instruction::POP << eth::Instruction::SWAP1;
+	// "return"
+	m_context << eth::Instruction::JUMP;
+
+	m_context << returnTag;
 	solAssert(m_context.getStackHeight() == stackHeightStart - 1, "");
 }
 

test/libsolidity/SolidityOptimizer.cpp

@@ -1004,6 +1004,38 @@ BOOST_AUTO_TEST_CASE(block_deduplicator)
 	BOOST_CHECK_EQUAL(pushTags.size(), 2);
 }
 
+BOOST_AUTO_TEST_CASE(block_deduplicator_loops)
+{
+	AssemblyItems input{
+		u256(0),
+		eth::Instruction::SLOAD,
+		AssemblyItem(PushTag, 1),
+		AssemblyItem(PushTag, 2),
+		eth::Instruction::JUMPI,
+		eth::Instruction::JUMP,
+		AssemblyItem(Tag, 1),
+		u256(5),
+		u256(6),
+		eth::Instruction::SSTORE,
+		AssemblyItem(PushTag, 1),
+		eth::Instruction::JUMP,
+		AssemblyItem(Tag, 2),
+		u256(5),
+		u256(6),
+		eth::Instruction::SSTORE,
+		AssemblyItem(PushTag, 2),
+		eth::Instruction::JUMP,
+	};
+	BlockDeduplicator dedup(input);
+	dedup.deduplicate();
+
+	set<u256> pushTags;
+	for (AssemblyItem const& item: input)
+		if (item.type() == PushTag)
+			pushTags.insert(item.data());
+	BOOST_CHECK_EQUAL(pushTags.size(), 1);
+}
+
 BOOST_AUTO_TEST_SUITE_END()
 
 }