Redundant store eliminator. by chriseth · Pull Request #11352 · argotorg/solidity

chriseth · 2021-05-05T16:15:59Z

No description provided.

chriseth · 2021-05-06T14:26:15Z

+
+	// TODO handle reverts of user-defined functions
+
+	// TODO is this all correct WRT termination side-effects of function


Probably not important because all we care about are ExpressionStatements.

chriseth · 2021-05-06T14:27:47Z

 object "C_12" {
    code {
        {
-            mstore(64, 128)


This removal is what most of the gas savings are about...

Will that still work with enabled stack-limit-evasion, with which this will have to become mstore(64, memoryguard(128)) and be propagated properly?

If we replace mstore(64, 128) with mstore(64, memoryguard(128)), then none of the mload(64) would be resolved. Which in turn means that this cannot be removed. I think we cannot use memory optimizations if we need to use stack limit evader.

chriseth · 2021-05-06T14:28:54Z

    code {
        {
            let _1 := 1
-            sstore(_1, _1)


chriseth · 2021-05-10T11:53:54Z

For this code, it would be very useful to somehow combine SideEffects and ControlFlowSideEffects, or at least make ControlFlowSideEffects available for user-defined functions instead of just for builtins.

cameel

Just a few initial remarks. I'm planning to do a full review later.

cameel · 2021-05-20T16:05:53Z

+		inline bool operator==(State _other) const { return m_value == _other.m_value; }
+		inline bool operator!=(State _other) const { return !operator==(_other); }
+		static inline void join(State& _a, State const& _b)


Does inline make any difference here? These should be considred inline by default.

Suggested change

inline bool operator==(State _other) const { return m_value == _other.m_value; }

inline bool operator!=(State _other) const { return !operator==(_other); }

static inline void join(State& _a, State const& _b)

bool operator==(State _other) const { return m_value == _other.m_value; }

bool operator!=(State _other) const { return !operator==(_other); }

static void join(State& _a, State const& _b)

cameel · 2021-05-20T16:14:35Z

+		static inline void join(State& _a, State const& _b)
+		{
+			// Using "max" works here because of the order of the values in the enum.
+			_a.m_value =  Value(std::max(int(_a.m_value), int(_b.m_value)));


Suggested change

_a.m_value = Value(std::max(int(_a.m_value), int(_b.m_value)));

_a.m_value = Value(std::max(int(_a.m_value), int(_b.m_value)));

cameel · 2021-05-20T16:21:15Z

+	/// Joins the assignment mapping of @a _source into @a _target according to the rules laid out
+	/// above.
+	/// Will destroy @a _source.
+	static void merge(TrackedStores& _target, TrackedStores&& _source);
+	static void merge(TrackedStores& _target, std::vector<TrackedStores>&& _source);


Which rules is this referring to? Was RedundantStoreEliminator meant to have a docstring describing them?

This is copied from redundant assign eliminator. I guess it is documented there. I still need to combine the code.

hrkrshnn · 2021-05-26T08:42:03Z

+		return false;
+	if (_covered.start && _covered.length && _covered.start == _covering.start && _covered.length == _covering.length)
+		return true;
+	if (_covered.location == Location::Storage && _covered.start && _covering.start && *_covered.start == *_covering.start)


Why not use knowledge base here to check equality?

Because the CSE will have done the work for us already.

hrkrshnn · 2021-05-26T11:09:13Z

+		if (optional<u256> startDiff = knowledge.differenceIsKnownConstant(*_covered.start, *_covering.start))
+			if (optional<u256> lengthDiff = knowledge.differenceIsKnownConstant(*_covering.length, *_covered.length))
+				if (*startDiff <= *lengthDiff && *startDiff <= largestPositive && *lengthDiff <= largestPositive)
+					return true;


Maybe you can add a test that would lead to an incorrect removal and mention EIP-1985?

{ let x := 0 calldatacopy(0, 0, 115792089237316195423570985008687907853269984665640564039457584007913129639935) mstore(x, 20) return(0, 32) } // ---- // step: redundantStoreEliminator // // { // let x := 0 // let _1 := 115792089237316195423570985008687907853269984665640564039457584007913129639935 // let _2 := 0 // let _3 := 0 // mstore(x, 20) // return(0, 32) // }

Interesting! So it seems there is a bug in my code?

I think it can have bugs like these when writing to locations beyond 2**128 - 1. This is a grey area because of EIP-1985.

chriseth · 2021-05-26T14:31:08Z

Rebased and added a feature to only ignore memory if msize is found.

axic · 2021-05-26T14:40:38Z

Rebased

You surely didn't rebase it to the head given all these conflicts displayed 😅

chriseth · 2021-05-26T17:29:01Z

Now rebased.

chriseth · 2021-08-11T16:59:59Z

Rebased this on top of the refactor.

chriseth · 2021-11-23T14:41:12Z

-//         sstore(0, length)
+//         mstore(64, newFreePtr)
+//         mstore(memPtr, length)
+//         extcodecopy(value, add(memPtr, 32), 0, length)


I hope we can prove memPtr + 32 <= memPtr + 32 with the linear solver soon...

chriseth · 2022-01-06T09:54:58Z

+		optional<u256> startDiff = knowledge.differenceIfKnownConstant(*_covered.start, *_covering.start);
+		optional<u256> coveredLength = knowledge.valueIfKnownConstant(*_covered.length);
+		optional<u256> lengthDiff = knowledge.differenceIfKnownConstant(*_covering.length, *_covered.length);
+		if (


There is no proof for this yet.

chriseth · 2022-01-06T10:04:47Z

Note that this step is less powerful than it could be due to the following reason:
Memory stores at the end of a function cannot be removed because we don't analyze the context in which the function is used so we mark all memory at the end of a function as potentially used.
What could improve this would be if we pull the return or stop instruction at the end of an external or public function into the function itself. Currently it is inside the dispatch routine and functions will likely not be inlined into the dispatch routine. It could help to create another function that is called from the dispatch routine and has

abi decode
call internal part of function
abi encode and return

This would also more nicely structure the code.

chriseth · 2022-01-06T10:34:32Z

-				if (*length2 <= *diff && *length2 <= largestPositive && *diff <= largestPositive)
-					return true;
-		}
+		// We use diff >= _op1.length && diff <= 2**256 - _op2.length


This is so annoying. This is not enough to remove the mstore in

mstore(0, 0) let length := calldataload(0) calldatacopy(0x20, 0x20, length) return(0x20, length)

Just to check that I understand this thing correctly (which I might not yet)... what currently happens is that both the mstore and the calldatacopy are Undecided when we hit the return and then the return does not know that it won't read from 0 because of the potential overflow?

Couldn't we when hitting the return walk the previous memory writes in backwards order and check for knownCovered writes and if we find one stop?

I.e. return(0x20, length) would first look at calldatacopy(0x20, 0x20, length), realize that its entire read is covered by that write and be satisfied and not even consider earlier writes, i.e. not even consider the mstore(0,0)...
Then the mstore(0,0) would remain Undecided and be removed, right?

I mean: the reason that we can remove mstore(0,0) is not that the return won't read from offset 0, but that it will only read what calldatacopy wrote, isn't it?

The order of writes is not preserved in m_stores, though, is it? Hm...

The reason is that knownUnrelated does not return true for the mstore(0, 0) and return(0x20, length).

That's one reason why we can remove mstore(0,0)... the other reason is that the memory read by return(0x20, length) is fully covered by calldatacopy(0x20, 0x20, length), so no earlier writes are relevant.

Ideally we'd be able to exploit both of those reasons...

If you had mstore(0x20, 0) instead, then return(0x20, length) would not be unrelated, but the mstore could still be removed.

I could actually imagine that that reason is the more useful one in general, since we'll probably rarely just read random stuff that wasn't written specifically... so in general, I think it will more often be the case that something only reads stuff covered by earlier writes rather than that the locations are known to be disjoint...

In general, though, we would have to track this across multiple writes - it's just a coincidence that it's a single write that covers the entire read in this example... i.e. in

mstore(y, 0x42) mstore(x, 0x20) mstore(add(x, 0x20), 0x20) return(x, 0x40)

I'd ideally want to recognize that the last two mstores together cover everything that's read by the return and be able to remove the first mstore without any knowledge about x or y... but yeah, just ideally, this may be infeasible to do...

Well, actually, probably not infeasible... but at least tricky :-).

Note from call: maybe it's worth differentiating between diff being positive or negative. So with an s256 diff, it may be

if (diff >= 0 && diff >= length1) return true; if (diff < 0 && -diff >= length2) return true;

But at least I haven't convinced myself yet that there's no other weird overflow cases to consider.

ekpyron · 2022-01-06T11:36:40Z

+			*instruction == Instruction::EXTCODECOPY ||
+			*instruction == Instruction::CODECOPY ||
+			*instruction == Instruction::CALLDATACOPY ||
+			*instruction == Instruction::RETURNDATACOPY ||
+			*instruction == Instruction::MSTORE ||
+			*instruction == Instruction::MSTORE8


Don't we have some abstract semantic property to cover this? Hm, in those "writing to memory" includes the call opcodes...

You are right, even if we don't have such a property yet, we should create it. "No side effects apart from writing to memory".

ekpyron · 2022-01-06T11:46:45Z

+	scheduleUnusedForDeletion();
+}
+
+vector<UnusedStoreEliminator::Operation> UnusedStoreEliminator::operationsFromFunctionCall(


I wonder whether we should move the main part of this to some abstract semantic property as well.
I.e. just have the builtins tell us "I'm reading the memory range [arg0, arg1)" instead of just "I'm reading memory".

It might be useful to have that information elsewhere and also if e.g. there's ever a new EVM opcode that accesses memory or storage, we'll definitely define its semantic properties, but it's much easier to miss editing stuff elsewhere like here.

Yep, true! We could use that routine in/as replacement for isSimpleLoad / isSimpleStore

chriseth · 2022-01-06T17:08:20Z

 //         sstore(not(gcd(_3, _2)), _1)
 //         sstore(2, _1)
 //         extcodecopy(_1, msize(), _1, _1)
+//         sstore(0, 0)


I have NO idea why this was missing before - maybe there is a really bad bug lurking here somewhere...

That's really a bit weird...

Or wait... did "before" have the equal store eliminator merged :-)? If so, then that's the reason I guess.

Yeah, I guess that's it. And the equal store eliminator removes the second copy, while the redundant store eliminator removes the first...

But even the equal store eliminator cannot remove this, can it?
It might be that the test results were just wrong for an earlier commit...

ok sure, but you cannot have both of them missing?

By the way, do we actually still need the equal store eliminator?

The diff I'm seeing merely moves the sstore(0, 0) three lines down...

The input contains two sstore(0, 0), the output contained one of them before the diff, and still contains one of them after the diff, it just removes the other one.

But yeah, I didn't get the chance to look at the equal store eliminator, but it did sound redundant to me relative to the redundant store eliminator :-).

chriseth · 2022-01-06T17:11:16Z

+
+k = Int('k')
+diff = Int('diff')
+solver.add(diff == start2 - start1 + k * 2**n_bits)


If we go with the assumption that all memory access is "small", this file needs to be updated to show that the condition is correct under that assumption.

chriseth · 2022-02-03T17:40:03Z

Do we want to split this into storage and memory and wait with the memory part until we have the linear solver?

ekpyron · 2022-02-03T21:24:33Z

Do we want to split this into storage and memory and wait with the memory part until we have the linear solver?

That may make sense, yes - if only, s.t. it's only one of the two to think about for the first version :-). We can also see what Fe says on Monday about the entire #11352 (comment) discussion...

chriseth · 2022-02-15T13:08:00Z

Replaced by #12672

chriseth commented May 5, 2021

View reviewed changes

Comment thread test/libyul/yulOptimizerTests/redundantStoreEliminator/remove_before_revert.yul Outdated

chriseth commented May 5, 2021

View reviewed changes

Comment thread test/libyul/yulOptimizerTests/redundantStoreEliminator/function_end.yul Outdated

chriseth force-pushed the redundantStoreEliminator branch 2 times, most recently from fd742a6 to 4231df3 Compare May 6, 2021 14:24

chriseth commented May 6, 2021

View reviewed changes

chriseth force-pushed the redundantStoreEliminator branch from 4231df3 to 76dfd1d Compare May 6, 2021 14:33

ekpyron self-requested a review May 6, 2021 14:35

chriseth force-pushed the redundantStoreEliminator branch 2 times, most recently from ff59f4e to 799d907 Compare May 10, 2021 11:51

chriseth force-pushed the redundantStoreEliminator branch from 799d907 to 49df2f2 Compare May 10, 2021 16:16

cameel reviewed May 20, 2021

View reviewed changes

hrkrshnn reviewed May 26, 2021

View reviewed changes

chriseth force-pushed the redundantStoreEliminator branch from 49df2f2 to a86689a Compare May 26, 2021 14:30

chriseth force-pushed the redundantStoreEliminator branch 2 times, most recently from ef143c8 to 0a57b2a Compare May 27, 2021 09:22

chriseth force-pushed the redundantStoreEliminator branch 2 times, most recently from 26c80b8 to b2d5289 Compare August 9, 2021 19:08

chriseth mentioned this pull request Aug 10, 2021

Refactor redundant assign #11774

Merged

chriseth force-pushed the redundantStoreEliminator branch from 792e9f0 to 573d3de Compare August 11, 2021 16:59

chriseth changed the base branch from develop to refactorRedundantAssign August 11, 2021 16:59

chriseth force-pushed the redundantStoreEliminator branch 2 times, most recently from 9758f5b to f9bc427 Compare August 12, 2021 09:17

chriseth commented Nov 23, 2021

View reviewed changes

Base automatically changed from renameRedundant to develop November 23, 2021 14:47

chriseth mentioned this pull request Nov 24, 2021

Extend DataFlowAnalyzer to not reset memory if memory ops use zero length #10645

Closed

cameel added the optimizer label Dec 10, 2021

chriseth mentioned this pull request Jan 3, 2022

Unexpected SLOADs during storing tightly packed struct #12460

Closed

ekpyron self-requested a review January 3, 2022 16:07

hrkrshnn mentioned this pull request Jan 6, 2022

Optimization for when entire contract has no code #12496

Closed

chriseth force-pushed the redundantStoreEliminator branch from 3b0c244 to 0d0802d Compare January 6, 2022 09:48

chriseth commented Jan 6, 2022

View reviewed changes

ekpyron reviewed Jan 6, 2022

View reviewed changes

chriseth commented Jan 6, 2022

View reviewed changes

chriseth added 10 commits February 15, 2022 11:03

Unused store eliminator.

1c3a5c7

chro

87e6e2f

Simplify condition.

e15cac6

Proof.

829f88e

Fix

f9b4b2f

Test updates after rule restrictions.

7641c52

Update changelog.

50743b4

Relax conditions.

0f47913

Update tests.

5fbf826

Gas updates.

fdee68f

chriseth force-pushed the redundantStoreEliminator branch from 4eebb71 to fdee68f Compare February 15, 2022 10:05

Add non-revert condition.

63c74f5


		// TODO handle reverts of user-defined functions

		// TODO is this all correct WRT termination side-effects of function

	_a.m_value = Value(std::max(int(_a.m_value), int(_b.m_value)));
	_a.m_value = Value(std::max(int(_a.m_value), int(_b.m_value)));

Uh oh!

Conversation

chriseth commented May 5, 2021

Uh oh!

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!

chriseth commented May 10, 2021

Uh oh!

cameel 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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!

chriseth commented May 26, 2021

Uh oh!

axic commented May 26, 2021

Uh oh!

chriseth commented May 26, 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!

chriseth commented Jan 6, 2022

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 Jan 6, 2022 • 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!

ekpyron Jan 6, 2022 •

edited

Loading