-
Notifications
You must be signed in to change notification settings - Fork 5.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Equal store eliminator. #12272
Equal store eliminator. #12272
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,70 @@ | ||
/* | ||
This file is part of solidity. | ||
|
||
solidity is free software: you can redistribute it and/or modify | ||
it under the terms of the GNU General Public License as published by | ||
the Free Software Foundation, either version 3 of the License, or | ||
(at your option) any later version. | ||
|
||
solidity is distributed in the hope that it will be useful, | ||
but WITHOUT ANY WARRANTY; without even the implied warranty of | ||
MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the | ||
GNU General Public License for more details. | ||
|
||
You should have received a copy of the GNU General Public License | ||
along with solidity. If not, see <http://www.gnu.org/licenses/>. | ||
*/ | ||
// SPDX-License-Identifier: GPL-3.0 | ||
/** | ||
* Optimisation stage that removes mstore and sstore operations if they store the same | ||
* value that is already known to be in that slot. | ||
*/ | ||
|
||
#include <libyul/optimiser/EqualStoreEliminator.h> | ||
|
||
#include <libyul/optimiser/CallGraphGenerator.h> | ||
#include <libyul/optimiser/OptimizerUtilities.h> | ||
#include <libyul/optimiser/Semantics.h> | ||
#include <libyul/AST.h> | ||
#include <libyul/Utilities.h> | ||
|
||
using namespace std; | ||
using namespace solidity; | ||
using namespace solidity::util; | ||
using namespace solidity::evmasm; | ||
using namespace solidity::yul; | ||
|
||
void EqualStoreEliminator::run(OptimiserStepContext const& _context, Block& _ast) | ||
{ | ||
EqualStoreEliminator eliminator{ | ||
_context.dialect, | ||
SideEffectsPropagator::sideEffects(_context.dialect, CallGraphGenerator::callGraph(_ast)) | ||
}; | ||
eliminator(_ast); | ||
|
||
StatementRemover remover{eliminator.m_pendingRemovals}; | ||
remover(_ast); | ||
} | ||
|
||
void EqualStoreEliminator::visit(Statement& _statement) | ||
{ | ||
// No need to consider potential changes through complex arguments since | ||
// isSimpleStore only returns something if the arguments are identifiers. | ||
if (ExpressionStatement const* expression = get_if<ExpressionStatement>(&_statement)) | ||
{ | ||
if (auto vars = isSimpleStore(StoreLoadLocation::Storage, *expression)) | ||
{ | ||
if (auto const* currentValue = valueOrNullptr(m_storage, vars->first)) | ||
if (*currentValue == vars->second) | ||
m_pendingRemovals.insert(&_statement); | ||
Comment on lines
+58
to
+59
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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)
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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah no, it's fine - we only replace variables by variables and we clear everything whenever there was an assignment to the variable. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What exactly do you want me to mention? These are all properties of the base class. |
||
} | ||
else if (auto vars = isSimpleStore(StoreLoadLocation::Memory, *expression)) | ||
{ | ||
if (auto const* currentValue = valueOrNullptr(m_memory, vars->first)) | ||
if (*currentValue == vars->second) | ||
m_pendingRemovals.insert(&_statement); | ||
} | ||
} | ||
|
||
DataFlowAnalyzer::visit(_statement); | ||
} |
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
@@ -0,0 +1,60 @@ | ||||||
/* | ||||||
This file is part of solidity. | ||||||
|
||||||
solidity is free software: you can redistribute it and/or modify | ||||||
it under the terms of the GNU General Public License as published by | ||||||
the Free Software Foundation, either version 3 of the License, or | ||||||
(at your option) any later version. | ||||||
|
||||||
solidity is distributed in the hope that it will be useful, | ||||||
but WITHOUT ANY WARRANTY; without even the implied warranty of | ||||||
MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the | ||||||
GNU General Public License for more details. | ||||||
|
||||||
You should have received a copy of the GNU General Public License | ||||||
along with solidity. If not, see <http://www.gnu.org/licenses/>. | ||||||
*/ | ||||||
// SPDX-License-Identifier: GPL-3.0 | ||||||
/** | ||||||
* Optimisation stage that removes mstore and sstore operations if they store the same | ||||||
* value that is already known to be in that slot. | ||||||
*/ | ||||||
|
||||||
#pragma once | ||||||
|
||||||
#include <libyul/optimiser/DataFlowAnalyzer.h> | ||||||
#include <libyul/optimiser/OptimiserStep.h> | ||||||
|
||||||
namespace solidity::yul | ||||||
{ | ||||||
|
||||||
/** | ||||||
* 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. | ||||||
cameel marked this conversation as resolved.
Show resolved
Hide resolved
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. But that is exactly what is written there, isn't it? We use this wording in all the other steps as well. |
||||||
* | ||||||
* Prerequisite: Disambiguator, ForLoopInitRewriter. | ||||||
*/ | ||||||
class EqualStoreEliminator: public DataFlowAnalyzer | ||||||
{ | ||||||
public: | ||||||
static constexpr char const* name{"EqualStoreEliminator"}; | ||||||
static void run(OptimiserStepContext const&, Block& _ast); | ||||||
|
||||||
private: | ||||||
EqualStoreEliminator( | ||||||
Dialect const& _dialect, | ||||||
std::map<YulString, SideEffects> _functionSideEffects | ||||||
): | ||||||
DataFlowAnalyzer(_dialect, std::move(_functionSideEffects)) | ||||||
{} | ||||||
|
||||||
protected: | ||||||
using ASTModifier::visit; | ||||||
void visit(Statement& _statement) override; | ||||||
|
||||||
std::set<Statement const*> m_pendingRemovals; | ||||||
}; | ||||||
|
||||||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -178,7 +178,7 @@ contract DepositContract is IDepositContract, ERC165 { | |
// compileViaYul: also | ||
// ---- | ||
// constructor() | ||
// gas irOptimized: 1558001 | ||
// gas irOptimized: 1557137 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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! |
||
// gas legacy: 2436584 | ||
// gas legacyOptimized: 1776483 | ||
// supportsInterface(bytes4): 0x0 -> 0 | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -38,12 +38,12 @@ contract c { | |
// compileViaYul: also | ||
// ---- | ||
// set(uint256): 7 -> true | ||
// gas irOptimized: 110011 | ||
// gas irOptimized: 110119 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Seems to be working 😅 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🤷 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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... There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. --- /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"
}
} There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The main change is that zero constants are now inlined, so I would say it's not an issue. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should perhaps implement evaluating |
||
// gas legacy: 110616 | ||
// gas legacyOptimized: 110006 | ||
// retrieve(uint256): 7 -> 1, 3, 4, 2 | ||
// copy(uint256,uint256): 7, 8 -> true | ||
// gas irOptimized: 118707 | ||
// gas irOptimized: 118698 | ||
// gas legacy: 119166 | ||
// gas legacyOptimized: 118622 | ||
// retrieve(uint256): 7 -> 1, 3, 4, 2 | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,25 @@ | ||
{ | ||
let a := calldataload(0) | ||
let b := 20 | ||
sstore(a, b) | ||
if calldataload(32) { | ||
sstore(a, b) | ||
pop(staticcall(0, 0, 0, 0, 0, 0)) | ||
sstore(a, b) | ||
} | ||
sstore(a, b) | ||
} | ||
// ==== | ||
// EVMVersion: >=byzantium | ||
// ---- | ||
// step: equalStoreEliminator | ||
// | ||
// { | ||
// let a := calldataload(0) | ||
// let b := 20 | ||
// sstore(a, b) | ||
// if calldataload(32) | ||
// { | ||
// pop(staticcall(0, 0, 0, 0, 0, 0)) | ||
// } | ||
// } |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,20 @@ | ||
{ | ||
let x := calldataload(0) | ||
let y := calldataload(1) | ||
|
||
sstore(x, y) | ||
for {let a := 1} lt(a, 10) {a := add(a, 1) } { | ||
sstore(x, y) | ||
} | ||
} | ||
// ---- | ||
// step: equalStoreEliminator | ||
// | ||
// { | ||
// let x := calldataload(0) | ||
// let y := calldataload(1) | ||
// sstore(x, y) | ||
// let a := 1 | ||
// for { } lt(a, 10) { a := add(a, 1) } | ||
// { sstore(x, y) } | ||
// } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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...There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Something like
extractStoreArguments
might be more understandable?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that removing
auto
might help.Too bad that a destructuring assignment like
auto [location, value] =
won't work here due to theoptional
.