Refactor redundant assign by chriseth · Pull Request #11774 · argotorg/solidity

chriseth · 2021-08-10T14:23:07Z

Needed for #11352

chriseth · 2021-08-11T09:33:21Z

@leonardoalt Is that an "approve"? :)

leonardoalt · 2021-08-11T09:38:21Z

not yet

ekpyron · 2021-08-11T07:30:48Z

-	ForLoopInfo outerForLoopInfo;
-	swap(outerForLoopInfo, m_forLoopInfo);
+	ScopedSaveAndRestore outerForLoopInfo(m_forLoopInfo, {});
 	++m_forLoopNestingDepth;


Suggested change

++m_forLoopNestingDepth;

ScopedSaveAndRestore loopNestingDepthIncrement(m_forLoopNestingDepth, m_forLoopNestingDepth + 1);

not sure that's really nicer, but it's at least more local than having the decrement at the very end. But also fine keeping as is.

ekpyron · 2021-08-11T07:32:24Z

+
+/// Working data for traversing for-loops.
+template <class TrackedAssignments>
+struct ForLoopInfo


That's quite a generic name for a top level construct... and also not very descriptive...

I know... Let me see what I can do.

chriseth · 2021-08-11T15:42:39Z

Did a complete makeover and create a common base class.

chriseth · 2021-08-11T15:43:56Z

Sorry, this is probably difficult to review, something like a three-way diff is probably the best way...

leonardoalt · 2021-08-12T11:05:50Z

+		Value m_value = Undecided;
+	};
+
+	using TrackedStores = std::map<YulString, std::map<Statement const*, State>>;


Might make sense to use a compare function on the inner map based on Statement id or something similar

You mean to ensure determinism? I don't think this is needed, to be honest. Furthermore, there are no IDs in yul.

Yea, because there was a TODO somewhere about determinism

YulString comparison is "weird", but deterministic. (Ah, sorry you meant the inner one... but still probably fine, I'll keep an eye on it)

leonardoalt · 2021-08-12T11:06:00Z

+	void finalize(YulString _variable, State _finalState);
+
+	Dialect const& m_dialect;
+	std::set<Statement const*> m_pendingRemovals;


Same here (re compare function)

ekpyron · 2021-08-13T10:40:37Z

 	}

-	for (auto const& assignment: assignments)
+	for (auto const& store: stores)


Suggested change

for (auto const& store: stores)

for (auto&& [statement, state]: stores)

?

considered this, but there is another local variable called state...

How can I enforce statement and state to be non-references? Is auto [statement, state] enough?

The condition below can just be

if ((state == State::Unused || (state == State::Undecided && _finalState == State::Unused)) && ...

or the inner one could just be renamed... but also fine keeping it.

Forcing auto [statement, state] to be of value type is something I wouldn't try myself :-). (The problem there not so much being auto, but the structured binding working via weird intermediate tuple types by spec ;-))

ekpyron · 2021-08-13T10:44:30Z

+		Value m_value = Undecided;
+	};
+
+	using TrackedStores = std::map<YulString, std::map<Statement const*, State>>;


YulString comparison is "weird", but deterministic. (Ah, sorry you meant the inner one... but still probably fine, I'll keep an eye on it)

ekpyron

Some more comments, but I'm rather confident that it does what it did before.

Why don't we remove multi-assignments? As in both: what prevents them to be removed in the code and why is removing them prevented at all...

ekpyron · 2021-08-13T10:46:46Z

+struct Dialect;
+
+/**
+ * Base class for both RedundantAsssignEliminator and RedundantStoreEliminator.


May state the prerequisites induced by the base here already as well?

Also says Ass_sign by the way ;-).

ekpyron · 2021-08-13T11:13:54Z

+*/
+// SPDX-License-Identifier: GPL-3.0
+/**
+ * Base class for both RedundantAsssignEliminator and RedundantStoreEliminator.


Suggested change

* Base class for both RedundantAsssignEliminator and RedundantStoreEliminator.

* Base class for both RedundantAssignEliminator and RedundantStoreEliminator.

Ass_sign is a nice typo :-).

ekpyron · 2021-08-13T11:22:45Z

+	std::map<Statement const*, State> stores;
+	util::joinMap(stores, std::move(m_stores[_variable]), State::join);


Isn't this just

std::map<Statement const*, State> stores = std::move(m_stores[_variable]);

?

Yes, not sure why I did it that way, maybe because of conformity? I can change it to use move it is easier to understand.

ekpyron · 2021-08-13T11:32:29Z

+		if (assignment->variableNames.size() == 1)
+			// Default-construct it in "Undecided" state if it does not yet exist.
+			m_stores[assignment->variableNames.front().name][&_statement];


It has always worked like that apparently, but I'm not entirely clear on why only single assignments are relevant here...

On the other hand: why exactly did this move, since it still only applies to the Assignment case?

Explanation about moving is here: https://github.com/ethereum/solidity/pull/11774/files#r688454105

single-assignments are fine because of SSA: If this is a multi-assignment and turns out to be redundant, the actual SSA assignment will be redundant and removed, while this assignment has to stay unless the right-hand-side is side-effect-free. We have to assign the value of the righ-hand-side somewhere, so this will turn into a variable declaration and the variable will not be used at all.

Yul needs a pop<n> :-).

And the reason for multi-assignments to generally not be removed by any of this here, is that they are never actually entered in the map in the first place? Might be worth an assert in finalize... but also fine.

ekpyron · 2021-08-13T11:48:41Z

 	}

-	for (auto const& assignment: assignments)
+	for (auto const& store: stores)


The condition below can just be

if ((state == State::Unused || (state == State::Undecided && _finalState == State::Unused)) && ...

or the inner one could just be renamed... but also fine keeping it.

Forcing auto [statement, state] to be of value type is something I wouldn't try myself :-). (The problem there not so much being auto, but the structured binding working via weird intermediate tuple types by spec ;-))

chriseth · 2021-08-13T12:00:09Z

Addressed all comments.

ekpyron

Can be squashed and merged.

chriseth requested review from christianparpart and ekpyron August 10, 2021 14:23

leonardoalt reviewed Aug 10, 2021

View reviewed changes

Comment thread libsolutil/CommonData.h

leonardoalt previously approved these changes Aug 11, 2021

View reviewed changes

ekpyron previously approved these changes Aug 11, 2021

View reviewed changes

chriseth dismissed stale reviews from ekpyron and leonardoalt via 66a4d8f August 11, 2021 15:42

chriseth force-pushed the refactorRedundantAssign branch from 427ed2c to 66a4d8f Compare August 11, 2021 15:42

chriseth force-pushed the refactorRedundantAssign branch 2 times, most recently from 956e763 to e30ab69 Compare August 11, 2021 16:18

chriseth requested a review from ekpyron August 12, 2021 09:15

leonardoalt reviewed Aug 12, 2021

View reviewed changes

chriseth force-pushed the refactorRedundantAssign branch from e30ab69 to f2d89b0 Compare August 13, 2021 08:03

ekpyron reviewed Aug 13, 2021

View reviewed changes

ekpyron previously approved these changes Aug 13, 2021

View reviewed changes

Refactor RedundantAssignEliminator.

3622b30

chriseth dismissed ekpyron’s stale review via 3622b30 August 13, 2021 12:25

chriseth force-pushed the refactorRedundantAssign branch from e098005 to 3622b30 Compare August 13, 2021 12:25

chriseth enabled auto-merge August 13, 2021 12:25

ekpyron approved these changes Aug 13, 2021

View reviewed changes

chriseth merged commit 07def75 into develop Aug 13, 2021

chriseth deleted the refactorRedundantAssign branch August 13, 2021 13:05

	++m_forLoopNestingDepth;
	ScopedSaveAndRestore loopNestingDepthIncrement(m_forLoopNestingDepth, m_forLoopNestingDepth + 1);

	for (auto const& store: stores)
	for (auto&& [statement, state]: stores)

	* Base class for both RedundantAsssignEliminator and RedundantStoreEliminator.
	* Base class for both RedundantAssignEliminator and RedundantStoreEliminator.

		std::map<Statement const*, State> stores;
		util::joinMap(stores, std::move(m_stores[_variable]), State::join);

Uh oh!

Conversation

chriseth commented Aug 10, 2021

Uh oh!

Uh oh!

chriseth commented Aug 11, 2021

Uh oh!

leonardoalt commented Aug 11, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chriseth commented Aug 11, 2021

Uh oh!

chriseth commented Aug 11, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ekpyron Aug 13, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ekpyron Aug 13, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ekpyron left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chriseth commented Aug 13, 2021

Uh oh!

ekpyron left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

ekpyron Aug 13, 2021 •

edited

Loading

ekpyron Aug 13, 2021 •

edited

Loading