Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Avoid unnecessary pops in evm code transform. #12759

Merged
merged 2 commits into from
Mar 15, 2022
Merged

Avoid unnecessary pops in evm code transform. #12759

merged 2 commits into from
Mar 15, 2022

Conversation

ekpyron
Copy link
Member

@ekpyron ekpyron commented Mar 9, 2022 •
edited
Loading

Not happy about this yet, but pushing it as draft nonetheless...
Doesn't work in all cases either as it stands (and adds too much junk for some reason)...

@ekpyron ekpyron force-pushed the evmCodeTransformAvoidPops branch 2 times, most recently from 69d8a68 to ab5242e Compare March 9, 2022 14:34
@ethereum ethereum deleted a comment from stackenbotten Mar 9, 2022
@ekpyron
Copy link
Member Author

ekpyron commented Mar 14, 2022

Gas differences:

Click for a table of gas differences
File name IR-optimized (%) Legacy-Optimized (%) Legacy (%)
constructor_inheritance_init_order.sol 0.333463 0 0
constructor_with_params_inheritance.sol 0.235661 0 0
constructor_with_params.sol 0.32917 0 0
constructor_inheritance_init_order_2.sol 0.333463 0 0
constructor_with_params_diamond_inheritance.sol -1.16682 0 0
smoke/constructor.sol -1.03936 0 0
events/event_dynamic_array_storage_v2.sol -0.00435745 0 0
events/event_emit_from_other_contract.sol 0.29745 0 0
events/event_dynamic_array_storage.sol -0.00435745 0 0
functionCall/failed_create.sol -0.890721 0 0
functionCall/creation_function_call_with_salt.sol -1.59748 0 0
functionCall/gas_and_value_basic.sol -2.41069 0 0
functionCall/external_call_to_nonexisting_debugstrings.sol -0.673481 0 0
functionCall/external_call_to_nonexisting.sol -3.34625 0 0
functionCall/mapping_array_internal_argument.sol -0.00446772 0 0
functionCall/creation_function_call_with_args.sol -1.59875 0 0
functionCall/send_zero_ether.sol -1.2792 0 0
functionCall/gas_and_value_brace_syntax.sol -2.41069 0 0
externalContracts/ramanujan_pi.sol -1.85385 0 0
externalContracts/prbmath_unsigned.sol -0.407828 0 0
externalContracts/deposit_contract.sol -0.489016 0 0
externalContracts/prbmath_signed.sol -0.57912 0 0
externalContracts/base64.sol -0.125895 0 0
externalContracts/FixedFeeRegistrar.sol 0.132601 0 0
externalContracts/strings.sol -0.481318 0 0
constructor/constructor_static_array_argument.sol -0.0120661 0 0
constructor/arrays_in_constructors.sol -1.49351 0 0
constructor/no_callvalue_check.sol 0.000833083 0 0
constructor/constructor_arguments_external.sol 0.434942 0 0
constructor/bytes_in_constructors_packer.sol -1.80531 0 0
constructor/bytes_in_constructors_unpacker.sol -1.50081 0 0
inheritance/value_for_constructor.sol -1.0109 0 0
inheritance/inherited_function_calldata_memory_interface.sol -8.63195 0 0
abiEncoderV2/abi_encode_calldata_slice.sol -0.0235775 0 0
abiEncoderV2/storage_array_encoding.sol -0.00885951 0 0
abiEncoderV2/abi_encode_v2_in_function_inherited_in_v1_contract.sol -0.00667117 0 0
abiEncoderV2/calldata_array.sol -0.00872895 0 0
abiEncoderV2/abi_encode_v2.sol -0.0194176 0 0
userDefinedValueType/erc20.sol -1.19612 0 0
userDefinedValueType/calldata.sol -0.00408717 0 0
libraries/internal_types_in_library.sol -0.000863051 0 0
libraries/using_library_mappings_public.sol -0.00167363 0 0
libraries/using_library_structs.sol -0.00982125 0 0
abiEncoderV1/abi_encode_calldata_slice.sol -0.0235775 0 0
abiEncoderV1/abi_decode_v2_storage.sol -0.00098371 0 0
abiEncoderV1/struct/struct_storage_ptr.sol 0.00775212 0 0
viaYul/array_storage_length_access.sol -0.000492669 0 0
viaYul/array_storage_index_boundary_test.sol -0.00935666 0 0
viaYul/array_memory_index_access.sol -0.00799157 0 0
viaYul/array_storage_index_access.sol -2.44065e-05 0 0
viaYul/array_storage_push_empty_length_address.sol -0.00061745 0 0
viaYul/array_storage_push_empty.sol -0.00161416 0 0
viaYul/array_storage_push_pop.sol -8.78286e-05 0 0
viaYul/copy_struct_invalid_ir_bug.sol -0.00265498 0 0
viaYul/array_storage_index_zeroed_test.sol -6.05808e-05 0 0
state/blockhash_basic.sol -1.3359 0 0
array/constant_var_as_array_length.sol -0.51672 0 0
array/function_array_cross_calls.sol -2.01918 0 0
array/reusing_memory.sol -0.182212 0 0
array/byte_array_transitional_2.sol -0.00126622 0 0
array/fixed_arrays_in_constructors.sol 0.013954 0 0
array/bytes_length_member.sol -0.00362191 0 0
array/dynamic_multi_array_cleanup.sol -0.0024483 0 0
array/dynamic_array_cleanup.sol -0.00057705 0 0
array/fixed_arrays_as_return_type.sol -1.38173 0 0
array/dynamic_arrays_in_storage.sol -0.00808596 0 0
array/arrays_complex_from_and_to_storage.sol -0.00895599 0 0
array/fixed_array_cleanup.sol -0.000644407 0 0
array/create_memory_array.sol -0.00153146 0 0
array/delete/bytes_delete_element.sol -0.00177074 0 0
array/push/array_push_struct_from_calldata.sol -0.0100934 0 0
array/push/array_push_nested_from_calldata.sol -0.00176588 0 0
array/push/byte_array_push_transition.sol -0.00113316 0 0
array/push/push_no_args_2d.sol -0.00961001 0 0
array/push/push_no_args_bytes.sol -0.00752947 0 0
array/copying/array_copy_storage_storage_static_static.sol -0.00465924 0 0
array/copying/array_copy_different_packing.sol -0.00188175 0 0
array/copying/array_copy_target_simple_2.sol -0.000730026 0 0
array/copying/array_nested_calldata_to_storage.sol -0.00443918 0 0
array/copying/memory_dyn_2d_bytes_to_storage.sol -0.00461837 0 0
array/copying/array_nested_memory_to_storage.sol -0.00523964 0 0
array/copying/array_copy_including_array.sol -7.66265e-05 0 0
array/copying/array_copy_target_simple.sol -0.00067849 0 0
array/copying/copy_function_internal_storage_array.sol -0.00241863 0 0
array/copying/array_of_function_external_storage_to_storage_dynamic.sol -0.00955393 0 0
array/copying/copy_byte_array_in_struct_to_storage.sol 0.012785 0 0
array/copying/array_of_function_external_storage_to_storage_dynamic_different_mutability.sol -0.00958387 0 0
array/copying/calldata_array_dynamic_to_storage.sol -0.00179919 0 0
array/copying/array_of_struct_memory_to_storage.sol -0.000839285 0 0
array/copying/copy_byte_array_to_storage.sol -0.00165119 0 0
array/copying/copy_removes_bytes_data.sol -0.00225492 0 0
array/copying/array_copy_nested_array.sol -0.000289769 0 0
array/copying/array_of_struct_calldata_to_storage.sol -0.00165261 0 0
array/copying/copying_bytes_multiassign.sol 0.0072102 0 0
array/copying/storage_memory_nested.sol -0.00192507 0 0
array/copying/storage_memory_packed_dyn.sol -0.00525762 0 0
array/copying/array_copy_storage_storage_dyn_dyn.sol -0.0089769 0 0
array/copying/storage_memory_nested_struct.sol -0.000685284 0 0
array/copying/array_copy_storage_storage_different_base.sol -0.000885128 0 0
array/copying/storage_memory_nested_from_pointer.sol -0.00192507 0 0
array/copying/bytes_storage_to_storage.sol -0.0031624 0 0
array/pop/array_pop_uint24_transition.sol -0.00376944 0 0
array/pop/byte_array_pop_masking_long.sol -0.00553031 0 0
array/pop/byte_array_pop_copy_long.sol -0.00365287 0 0
array/pop/byte_array_pop_long_storage_empty_garbage_ref.sol -0.000701066 0 0
array/pop/byte_array_pop_long_storage_empty.sol -0.00203516 0 0
array/pop/array_pop_array_transition.sol -0.000264195 0 0
immutable/use_scratch.sol -0.595229 0 0
immutable/multi_creation.sol -0.642339 0 0
structs/struct_delete_storage_with_array.sol -0.00411103 0 0
structs/memory_structs_nested_load.sol -0.00628186 0 0
structs/struct_copy.sol -0.00481101 0 0
structs/struct_memory_to_storage_function_ptr.sol -0.00180432 0 0
structs/conversion/recursive_storage_memory.sol -0.0010309 0 0
structs/calldata/calldata_struct_with_nested_array_to_storage.sol -0.00196751 0 0
various/staticcall_for_view_and_pure.sol -3.04758e-06 0 0
various/erc20.sol -1.20742 0 0
various/senders_balance.sol -1.86886 0 0
various/destructuring_assignment.sol -0.00207756 0 0
various/value_complex.sol -0.396342 0 0
various/address_code.sol -2.50647 0 0
various/contract_binary_dependencies.sol -1.28194 0 0
various/value_insane.sol -0.612212 0 0
various/skip_dynamic_types_for_structs.sol -0.00447397 0 0
isoltestTesting/balance_other_contract.sol -2.11231 0 0

Diff of all regressions: https://gist.github.com/ekpyron/4fee7493fcf1f43088f1a7082d074867

@ekpyron
Copy link
Member Author

ekpyron commented Mar 14, 2022

Diff of external benchmarks between develop and this PR: https://gist.github.com/ekpyron/6f22ccabbed99b96c78dc0f9ec61624b

@ekpyron ekpyron force-pushed the evmCodeTransformAvoidPops branch 3 times, most recently from f19e401 to 0e6021a Compare March 14, 2022 20:47
@ekpyron ekpyron marked this pull request as ready for review March 14, 2022 20:54
chriseth
chriseth reviewed Mar 15, 2022
View reviewed changes
libevmasm/PeepholeOptimiser.cpp
@@ -117,6 +117,35 @@ struct OpPop: SimplePeepholeOptimizerMethod<OpPop>
}
};

struct OpStop: SimplePeepholeOptimizerMethod<OpStop>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this not handled by other steps?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Apparently not...
Ideally, this wouldn't be needed, but apparently some cases still slip through the logic below... and it generally doesn't hurt to have this.
It fixes the regression on the FixedRegistrar semantics test and saves ~2% of code size there apparently... this warrants a closer look at some point, but for now the peephole optimizer seems like a good fallback mechanism.

chriseth
chriseth reviewed Mar 15, 2022
View reviewed changes
test/cmdlineTests/viair_subobject_optimization/output
@@ -134,7 +126,9 @@ sub_0: assembly {
tag_4
jumpi
pop
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This could be removed as well :)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, I know - not entirely sure why it isn't yet. Either the logic in the rest of the PR misses some cases or this only comes into existence during evmasm optimization...

@ekpyron
Copy link
Member Author

ekpyron commented Mar 15, 2022

FYI this doesn't seem to measurably affect compile times on semanticTest/externalContracts.

ekpyron
ekpyron commented Mar 15, 2022
View reviewed changes
libyul/backends/evm/ControlFlowGraphBuilder.cpp Outdated
visited.insert(_u);
depths[_u] = low[_u] = _depth;

vector<CFG::BasicBlock*> children;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
vector<CFG::BasicBlock*> children;
vector<CFG::BasicBlock*> children = _u->entries;

@chriseth This I think I should still change... without this, I'm visiting the graph as directed graph, but the algorithm assumes it's undirected... it does affect gas costs, sometimes a bit better, sometimes a bit worse... I'm a bit surprised that it works both ways, but also visiting the entries here is probably the safer choice - although it may be worth thinking through this a bit more at some point...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe someone should try to understand the algorithm...

@ekpyron ekpyron force-pushed the evmCodeTransformAvoidPops branch 2 times, most recently from 075af6f to faaf0b9 Compare March 15, 2022 13:24
This was referenced Mar 15, 2022
Merged
Merged
chriseth
chriseth reviewed Mar 15, 2022
View reviewed changes
test/cmdlineTests/viair_subobject_optimization/output
@@ -134,7 +126,9 @@ sub_0: assembly {
tag_4
jumpi
pop
jump(tag_3)
0x00
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why does it not deduplicate anymore?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not entirely sure.
But I find it suspicious that the evmasm optimiser first inlines, then removes unused tags and then deduplicates (which relies on stuff being tagged, if I read it correctly)... I guess there's something off with that there.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is true. I think the deduplicator even ignores additional tags when it compares for equality.

@chriseth chriseth merged commit 724af73 into develop Mar 15, 2022
@chriseth chriseth deleted the evmCodeTransformAvoidPops branch March 15, 2022 18:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@chriseth chriseth chriseth approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@ekpyron @chriseth