Equal store eliminator. #12272

chriseth · 2021-11-11T15:16:34Z

This removes subsequent stores to the same location with the same value, even if reads happen in between.

chriseth · 2021-11-11T15:16:52Z

Partly solves #12211

axic · 2021-11-11T17:26:16Z

test/libsolidity/semanticTests/structs/struct_copy.sol

@@ -38,12 +38,12 @@ contract c {
 // compileViaYul: also
 // ----
 // set(uint256): 7 -> true
-// gas irOptimized: 110011
+// gas irOptimized: 110119


Seems to be working 😅

The assembly is actually shorter. My guess is that the removed mstore instructions makes it impossible for the opcode-based optimizer to remove the multiple keccak calls...

--- /tmp/ir_old.yul 2022-01-03 15:46:30.242337855 +0100 +++ /tmp/ir_opt.yul 2022-01-03 15:41:35.197221133 +0100 @@ -79,34 +79,26 @@ mstore(_1, var_k) mstore(0x20, _1) sstore(keccak256(_1, 0x40), 0x01) - mstore(_1, var_k) - mstore(0x20, _1) sstore(add(keccak256(_1, 0x40), 0x01), 0x03) - mstore(_1, var_k) - mstore(0x20, _1) sstore(add(keccak256(_1, 0x40), 2), 0x04) - mstore(_1, var_k) - mstore(0x20, _1) sstore(add(keccak256(_1, 0x40), 0x03), 2) var := 0x01 } function fun_copy(var_from, var_to) -> var_ { - let _1 := 0x00 - mstore(_1, var_from) - mstore(0x20, _1) - let dataSlot := keccak256(_1, 0x40) - mstore(_1, var_to) - mstore(0x20, _1) - let dataSlot_1 := keccak256(_1, 0x40) + mstore(0x00, var_from) + mstore(0x20, 0x00) + let dataSlot := keccak256(0x00, 0x40) + mstore(0x00, var_to) + let dataSlot_1 := keccak256(0x00, 0x40) if iszero(eq(dataSlot_1, dataSlot)) { sstore(dataSlot_1, sload(dataSlot)) let memberSlot := add(dataSlot_1, 1) - let _2 := add(dataSlot, 1) - if iszero(eq(memberSlot, _2)) + let _1 := add(dataSlot, 1) + if iszero(eq(memberSlot, _1)) { - sstore(memberSlot, sload(_2)) + sstore(memberSlot, sload(_1)) sstore(add(dataSlot_1, 2), sload(add(dataSlot, 2))) } sstore(add(dataSlot_1, 3), sload(add(dataSlot, 3))) @@ -119,18 +111,12 @@ mstore(_1, var_k) mstore(0x20, _1) var_a := sload(keccak256(_1, 0x40)) - mstore(_1, var_k) - mstore(0x20, _1) var_x := sload(add(keccak256(_1, 0x40), 1)) - mstore(_1, var_k) - mstore(0x20, _1) var_y := sload(add(keccak256(_1, 0x40), 2)) - mstore(_1, var_k) - mstore(0x20, _1) var_c := sload(add(keccak256(_1, 0x40), 3)) } } - data ".metadata" hex"a36469706673582212201575720a9612338487e4ea44beef0f99559f8ddb767d3b3a8f88b30e8203f9ee6c6578706572696d656e74616cf564736f6c634300080c0041" + data ".metadata" hex"a36469706673582212201300a8828ade0dfe77d7dbd65f00b07037165a285fa1ba1a0c986a59b74ec9b66c6578706572696d656e74616cf564736f6c634300080c0041" } }

The main change is that zero constants are now inlined, so I would say it's not an issue.

Should perhaps implement evaluating keccak for multi words (at least 2 words).

Marenz · 2021-11-22T13:09:26Z

libyul/optimiser/EqualStoreEliminator.cpp

+using namespace solidity::evmasm;
+using namespace solidity::yul;
+
+void EqualStoreEliminator::run(OptimiserStepContext& _context, Block& _ast)


As far as I can tell we only access the const member dialect for context. Should the parameter be a const ref maybe?

This is a standard interface, but I can check.

Marenz · 2021-11-22T13:13:39Z

libyul/optimiser/EqualStoreEliminator.cpp

+	// isSimpleStore only returns something if the arguments are identifiers.
+	if (ExpressionStatement const* expression = get_if<ExpressionStatement>(&_statement))
+	{
+		if (auto vars = isSimpleStore(StoreLoadLocation::Storage, *expression))


Not related to the PR changes, but the name isSimpleStore is very confusing given that it returns two variables. Without reading the docs to that function I'd have no idea what's going on here...

Something like extractStoreArguments might be more understandable?

I think that removing auto might help.

Too bad that a destructuring assignment like auto [location, value] = won't work here due to the optional.

Marenz · 2021-11-22T13:23:19Z

libyul/optimiser/EqualStoreEliminator.h

+	using ASTModifier::visit;
+	void visit(Statement& _statement) override;
+
+	void tryResolve(


Are the functions tryResolve and tryEvaluateKecak implemented and called anywhere?

Ah, I think that was a copy-past-leftover.

cameel

Documentation for the new step is missing.

Also maybe a few more tests beyond the one from the issue might be useful. We can see the effect in full suite tests but I think this also needs some dedicated ones.

libyul/optimiser/EqualStoreEliminator.h

cameel · 2021-11-22T13:47:36Z

libyul/optimiser/EqualStoreEliminator.cpp

+				if (*currentValue == vars->second)
+					m_pendingRemovals.insert(&_statement);


Isn't this condition a bit too simple?

The docstring says that this is best used when there are no literal arguments. So let's take this code:

mstore(0, 0) mstore(0, 0)

ExpressionSplitter seems to remove literal arguments and it turns the code to this:

let _1 := 0 let _2 := 0 mstore(_2, _1) let _3 := 0 let _4 := 0 mstore(_4, _3)

but it this would defeat this optimization.

Yes, you also need to call the CSE to make this work properly, but I'm actually currently wondering whether we check somewhere that the value is an ssa variable with a movable value, this should be another condition.

Ah no, it's fine - we only replace variables by variables and we clear everything whenever there was an assignment to the variable.

Can you mention it in the docstring? A comment in this function would not hurt either. This kind of assumption is not easy to notice other than from the fact that the code looks way too simple for that it does so there must be some "catch" somewhere.

What exactly do you want me to mention? These are all properties of the base class.

cameel · 2021-11-22T13:53:37Z

test/libyul/YulOptimizerTestCommon.cpp

+		{"equalStoreEliminator", [&]() {
+			disambiguate();
+			EqualStoreEliminator::run(*m_context, *m_ast);
+		}},


ForLoopInitRewriter is listed in the docstring as a dependency too.

cameel · 2021-11-23T17:21:16Z

libyul/optimiser/EqualStoreEliminator.h

+ * Optimisation stage that removes mstore and sstore operations if they store the same
+ * value that is already known to be in that slot.
+ *
+ * Works best if the code is in SSA form - without literal arguments.


I'd make it more explicit. "Works best" does not really tell me what I can expect if my code does not satisfy this.

Suggested change

* Works best if the code is in SSA form - without literal arguments.

* Does not optimize parts of code with variables not in SSA form or with calls with literal arguments.

But that is exactly what is written there, isn't it? We use this wording in all the other steps as well.

hrkrshnn

Looks good. I'll commit a couple of tests that I wrote locally while testing this.

Edit: also updated the failing chromosome test.

hrkrshnn · 2022-01-03T09:45:59Z

libyul/optimiser/UnusedStoreBase.cpp

@@ -23,6 +23,7 @@

 #include <libyul/optimiser/Semantics.h>
 #include <libyul/optimiser/OptimiserStep.h>
+#include <libyul/optimiser/OptimizerUtilities.h>


Suggested change

#include <libyul/optimiser/OptimizerUtilities.h>

Actually, will update this myself.

hrkrshnn · 2022-01-03T12:04:15Z

test/libsolidity/semanticTests/externalContracts/deposit_contract.sol

@@ -178,7 +178,7 @@ contract DepositContract is IDepositContract, ERC165 {
 // compileViaYul: also
 // ----
 // constructor()
-// gas irOptimized: 1558001
+// gas irOptimized: 1557137


diff -u /tmp/develop.yul /tmp/eq.yul --- /tmp/develop.yul 2022-01-03 17:32:01.880093589 +0530 +++ /tmp/eq.yul 2022-01-03 17:31:28.243155455 +0530 @@ -546,7 +546,6 @@ calldatacopy(pos, value0, value1) let _1 := add(pos, value1) mstore(_1, 0) - mstore(_1, 0) end := add(_1, 16) } function calldata_array_index_range_access_bytes_calldata_3157(offset, length) -> offsetOut, lengthOut @@ -572,8 +571,6 @@ let _1 := add(pos, value1) mstore(_1, /** @src 0:6663:6664 "0" */ 0x00) /// @src 0:4559:9399 "contract DepositContract is IDepositContract, ERC165 {..." - mstore(_1, /** @src 0:6663:6664 "0" */ 0x00) - /// @src 0:4559:9399 "contract DepositContract is IDepositContract, ERC165 {..." end := add(_1, 32) } function abi_encode_packed_bytes32_bytes_calldata(pos, value0, value1, value2) -> end @@ -937,7 +934,7 @@ mstore8(memory_array_index_access_bytes_3175(memPtr), /** @src 0:9377:9390 "bytesValue[0]" */ byte(/** @src -1:-1:-1 */ 0, /** @src 0:9377:9390 "bytesValue[0]" */ _1)) } } - data ".metadata" hex"a364697066735822122006c8a10f3f04d8c69b52af5dbc3a82224acdfaa0224b0f3ed0cf38c99a4f0c7e6c6578706572696d656e74616cf564736f6c637823302e382e31322d63692e323032312e362e32332b636f6d6d69742e36383439373734620062" + data ".metadata" hex"a3646970667358221220a5a5d6e1a1822c1fcf45d9e6ac103252bac21d4af4c04b1a690185ad3147faf46c6578706572696d656e74616cf564736f6c63782b302e382e31322d646576656c6f702e323032322e312e332b636f6d6d69742e33633634333238612e6d6f64006a" } } Diff finished. Mon Jan 3 17:32:12 2022

Here's the diff!

hrkrshnn

Going to fix it myself.

hrkrshnn · 2022-01-04T05:28:34Z

libyul/optimiser/EqualStoreEliminator.cpp

+#include <libevmasm/GasMeter.h>
+#include <libsolutil/Keccak256.h>


Suggested change

#include <libevmasm/GasMeter.h>

#include <libsolutil/Keccak256.h>

hrkrshnn · 2022-01-04T05:29:32Z

libyul/optimiser/EqualStoreEliminator.cpp

+#include <libevmasm/GasMeter.h>
+#include <libsolutil/Keccak256.h>
+
+#include <limits>


Suggested change

#include <limits>

hrkrshnn · 2022-01-04T05:30:17Z

libyul/optimiser/EqualStoreEliminator.cpp

+#include <libyul/optimiser/CallGraphGenerator.h>
+#include <libyul/optimiser/OptimizerUtilities.h>
+#include <libyul/optimiser/Semantics.h>
+#include <libyul/optimiser/CallGraphGenerator.h>


Suggested change

#include <libyul/optimiser/CallGraphGenerator.h>

hrkrshnn · 2022-01-04T05:31:51Z

libyul/optimiser/EqualStoreEliminator.cpp

+#include <libyul/optimiser/OptimizerUtilities.h>
+#include <libyul/optimiser/Semantics.h>
+#include <libyul/optimiser/CallGraphGenerator.h>
+#include <libyul/SideEffects.h>


Suggested change

#include <libyul/SideEffects.h>

hrkrshnn

LGTM! Added a couple more tests and removed some more unused headers in the last commit. Feel free to squash the commits.

Would be nice to get a yes from @bshastry before merging, with regard to changes in fuzzer.

bshastry

LGTM wrt fuzzer once #12276 is merged

chriseth mentioned this pull request Nov 11, 2021

Why is struct_copy.sol so badly optimized? #12211

Closed

chriseth force-pushed the equalStoreEliminator branch 2 times, most recently from cc466dc to a528ffa Compare November 11, 2021 17:23

axic reviewed Nov 11, 2021

View reviewed changes

Marenz suggested changes Nov 22, 2021

View reviewed changes

cameel reviewed Nov 22, 2021

View reviewed changes

This was referenced Nov 22, 2021

Redundant store eliminator. #11352

Closed

Re-check if the identity precompile should be used for memcopy #12127

Closed

cameel reviewed Nov 23, 2021

View reviewed changes

chriseth force-pushed the equalStoreEliminator branch from dfed1e0 to 4dde26a Compare November 24, 2021 09:15

cameel mentioned this pull request Nov 26, 2021

External calls should not generate RETURNDATACOPY if the return data is not used #12306

Open

cameel added the optimizer label Dec 10, 2021

hrkrshnn reviewed Jan 3, 2022

View reviewed changes

hrkrshnn force-pushed the equalStoreEliminator branch 4 times, most recently from 3c64328 to 8a4821e Compare January 3, 2022 12:03

hrkrshnn reviewed Jan 3, 2022

View reviewed changes

chriseth added 3 commits January 3, 2022 15:52

Equal store eliminator.

772e100

Test updates.

e7fc2a1

Documentation

b354c81

chriseth force-pushed the equalStoreEliminator branch from 8a4821e to a88709c Compare January 3, 2022 14:52

chriseth mentioned this pull request Jan 3, 2022

Unexpected SLOADs during storing tightly packed struct #12460

Closed

hrkrshnn reviewed Jan 4, 2022

View reviewed changes

hrkrshnn force-pushed the equalStoreEliminator branch from a88709c to 40d2719 Compare January 4, 2022 05:41

hrkrshnn previously approved these changes Jan 4, 2022

View reviewed changes

hrkrshnn dismissed their stale review via a60bef5 January 4, 2022 06:30

hrkrshnn force-pushed the equalStoreEliminator branch from 40d2719 to a60bef5 Compare January 4, 2022 06:30

Added more tests. Updated chromosome. Remove unused headers.

85d1a57

hrkrshnn force-pushed the equalStoreEliminator branch from a60bef5 to 85d1a57 Compare January 4, 2022 07:57

bshastry reviewed Jan 4, 2022

View reviewed changes

chriseth merged commit c16867c into develop Jan 5, 2022

chriseth deleted the equalStoreEliminator branch January 5, 2022 10:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Equal store eliminator. #12272

Equal store eliminator. #12272

chriseth commented Nov 11, 2021

chriseth commented Nov 11, 2021

axic Nov 11, 2021

chriseth Nov 11, 2021

chriseth Nov 11, 2021

chriseth Jan 3, 2022

chriseth Jan 3, 2022

hrkrshnn Jan 4, 2022

Marenz Nov 22, 2021

chriseth Nov 23, 2021

Marenz Nov 22, 2021

Marenz Nov 22, 2021

cameel Nov 22, 2021

Marenz Nov 22, 2021

chriseth Nov 23, 2021

cameel left a comment •

edited

Loading

cameel Nov 22, 2021

chriseth Nov 23, 2021

chriseth Nov 23, 2021

cameel Nov 23, 2021

chriseth Nov 24, 2021

cameel Nov 22, 2021

cameel Nov 23, 2021

chriseth Nov 24, 2021

hrkrshnn left a comment •

edited

Loading

hrkrshnn Jan 3, 2022

hrkrshnn Jan 3, 2022

hrkrshnn Jan 3, 2022

hrkrshnn left a comment

hrkrshnn Jan 4, 2022

hrkrshnn Jan 4, 2022

hrkrshnn Jan 4, 2022

hrkrshnn Jan 4, 2022

hrkrshnn left a comment •

edited

Loading

bshastry left a comment

		if (*currentValue == vars->second)
		m_pendingRemovals.insert(&_statement);

	* Works best if the code is in SSA form - without literal arguments.
	* Does not optimize parts of code with variables not in SSA form or with calls with literal arguments.

		#include <libevmasm/GasMeter.h>
		#include <libsolutil/Keccak256.h>

Equal store eliminator. #12272

Equal store eliminator. #12272

Conversation

chriseth commented Nov 11, 2021

chriseth commented Nov 11, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cameel left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hrkrshnn left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hrkrshnn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hrkrshnn left a comment • edited Loading

Choose a reason for hiding this comment

bshastry left a comment

Choose a reason for hiding this comment

cameel left a comment •

edited

Loading

hrkrshnn left a comment •

edited

Loading

hrkrshnn left a comment •

edited

Loading