solidity
solidity copied to clipboard
ethereum
Optimize ``return(x,0) -> pop(x) return(0,0)`` (and also for ``revert``).
Nothing fancy, but a few gas here and there... Kind of depends on https://github.com/ethereum/solidity/issues/12913, but wouldn't be fatal to merge without.
Is there an optimizer step that might be more fitting? Structural sounds more like it operates on the structure of the code...
Is there an optimizer step that might be more fitting? Structural sounds more like it operates on the structure of the code...
I didn't see anything obvious... the expression simplifier only acts on expressions and can't easily add the pop... and the structural simplifier already has the literal rematerializer as soft-requirement, s.t. we likely get the 0 as literal...
We could extend the expression simplifier to expression statements instead I guess...
I didn't see anything obvious... the expression simplifier only acts on expressions and can't easily add the
pop... and the structural simplifier already has the literal rematerializer as soft-requirement, s.t. we likely get the0as literal... We could extend the expression simplifier to expression statements instead I guess...
Or start a new one?
Actually the ExpressionSimplifier is suitable after all - we already restrict simplification to variables to avoid side-effect issues anyways, so we can also just do that for this (and then don't need to insert a pop at all).
Weird that this change mostly increases gas -- or is that only for the isolated test cases and when it is used in the complete optimisation suite it helps?
Weird that this change mostly increases gas -- or is that only for the isolated test cases and when it is used in the complete optimisation suite it helps?
This is the gas diffs on our semantics test suite
Click for a table of gas differences
| File name | IR-optimized (%) | Legacy-Optimized (%) | Legacy (%) |
|---|---|---|---|
| events/event_indexed_string.sol | -0.000899607 | 0 | 0 |
| events/event_dynamic_nested_array_storage_v2.sol | -0.00162047 | 0 | 0 |
| events/event_dynamic_array_storage_v2.sol | -0.00264278 | 0 | 0 |
| events/event_emit_from_other_contract.sol | -0.64311 | 0 | 0 |
| events/event_dynamic_array_storage.sol | -0.00264278 | 0 | 0 |
| functionCall/gas_and_value_basic.sol | -0.164804 | 0 | 0 |
| functionCall/gas_and_value_brace_syntax.sol | -0.164804 | 0 | 0 |
| externalContracts/base64.sol | 0.000561615 | 0 | 0 |
| externalContracts/FixedFeeRegistrar.sol | 1.04122 | 0 | 0 |
| abiEncoderV2/abi_encode_calldata_slice.sol | -0.00407541 | 0 | 0 |
| userDefinedValueType/calldata.sol | -0.0139169 | 0 | 0 |
| abiEncoderV1/abi_encode_calldata_slice.sol | -0.00407541 | 0 | 0 |
| viaYul/array_storage_index_access.sol | -7.3568e-05 | 0 | 0 |
| viaYul/array_storage_push_empty.sol | -0.000546533 | 0 | 0 |
| viaYul/copy_struct_invalid_ir_bug.sol | 0.0176993 | 0 | 0 |
| viaYul/array_storage_index_zeroed_test.sol | -0.000181742 | 0 | 0 |
| array/dynamic_array_cleanup.sol | -0.000577349 | 0 | 0 |
| array/dynamic_arrays_in_storage.sol | -0.00179453 | 0 | 0 |
| array/fixed_array_cleanup.sol | -0.00751977 | 0 | 0 |
| array/push/nested_bytes_push.sol | -0.00167436 | 0 | 0 |
| array/push/push_no_args_2d.sol | -0.00524586 | 0 | 0 |
| array/push/push_no_args_bytes.sol | -0.0016341 | 0 | 0 |
| array/copying/array_copy_storage_storage_dyn_dyn.sol | -0.00538725 | 0 | 0 |
| array/pop/byte_array_pop_long_storage_empty_garbage_ref.sol | -0.00210321 | 0 | 0 |
| structs/struct_delete_storage_with_array.sol | -0.00246589 | 0 | 0 |
| structs/structs.sol | 0.00223159 | 0 | 0 |
| various/contract_binary_dependencies.sol | -0.641184 | 0 | 0 |
| various/swap_in_storage_overwrite.sol | -0.00273488 | 0 | 0 |
not sure that qualifies as "mostly increases gas" :-). But I'm just looking at the FixedFeeRegistrar.sol case... what's happening there is that due to the change, two tags in the final assembly end up slightly different, while they were the same before (allowing deduplication) causing a slightly larger code size... (We should also really distinguish code size from runtime gas cost in our tests...)
So, the gas differences are little, but still usually favorable, I'd say, apart from corner cases like that.
Unfortunately, there's some issue with some external test apparently, so so far I'm not getting gas diffs on the external projects, that'd be more interesting.
ir-optimize-evm+yul
| project | bytecode_size | deployment_gas | method_gas |
|---|---|---|---|
| bleeps | -0.63% ✅ |
0% |
-0% |
| brink | -0.02% ✅ |
||
| chainlink | -0.2% ✅ |
-0.23% ✅ |
-0.03% ✅ |
| colony | 0% |
||
| elementfi | +0% |
||
| ens | 0% |
-0.19% ✅ |
+0% |
| euler | -0.1% ✅ |
-0.1% ✅ |
+0.06% ❌ |
| gnosis | -0.05% ✅ |
-0% |
+0% |
| gp2 | +0.09% ❌ |
+0.04% ❌ |
+0.01% ❌ |
| perpetual-pools | -0.06% ✅ |
-0.06% ✅ |
+0.01% ❌ |
| pool-together | -0.21% ✅ |
-0.23% ✅ |
-0% |
| prb-math | 0% |
+0% |
0% |
| trident | -0.11% ✅ |
-0.16% ✅ |
-0.02% ✅ |
| uniswap | -0.12% ✅ |
+0.06% ❌ |
-0.04% ✅ |
| yield_liquidator | -0.24% ✅ |
-0.24% ✅ |
-0.01% ✅ |
| zeppelin | !V |
!V |
!V |
Yeah... I'd call that "mixed, slightly favorable" or so :-).
I also re-ran the tests and got the same problems, so the rebase didn't change anything in that regard
I'd say it's fine to merge given https://github.com/ethereum/solidity/pull/12762#discussion_r900162365
I'll rebase it in any case. But yeah, it's basically stuck on missing review.
Rebased... and well, it has been marked priority review for over a month...