solidity Mask generator optimizations

implements #15870

Mar 12 '25 16:03 moh-eulith

Thank you for your contribution to the Solidity compiler! A team member will follow up shortly.

If you haven't read our contributing guidelines and our review checklist before, please do it now, this makes the reviewing process and accepting your contribution smoother.

If you have any questions or need our help, feel free to post them in the PR or talk to us directly on the #solidity-dev channel on Matrix.

Mar 12 '25 16:03 github-actions[bot]

Looking at the build failures... I've been running ./test/soltest -p, and that passes. I'll start with the required ones.

Mar 12 '25 17:03 moh-eulith

Similarly test/cmdlineTests.sh --no-smt --update will update the command line tests (we'll then need to sanity-check the changes). The t_native_text_ext_* failures seem to be a transient network issue which will hopefully vanish on the next run. (Since you're not modifying the smtsolver, which require Z3 to be installed and are slow to run running with --no-smt should be enough)

Mar 12 '25 17:03 ekpyron

One option for testing would be to borrow from https://github.com/ethereum/solidity/blob/340ae32d5c27c87ce51ca0b484f7566f1951c4a0/test/libevmasm/Optimiser.cpp#L1333 while specifically running the ConstantOptimisationMethod::optimiseConstants on Assembly as constructed there. A few simple cases for masks in the middle bits, the high bits, and the low bits and maybe a non-mask should do it. If there's problems with that let us know!

Mar 12 '25 17:03 ekpyron

We're also wondering how best to add a unit tests for this and may get back to you with a suggestion for that.

We talked about it a bit and we could generally use a new test case type to test EVM assembly import and assembly-level optimizations. I'm going to work on this and then ping you here when you'll be able to use this.

It will work like --import-asm-json, just in a text format that's not as verbose as JSON.

Mar 12 '25 18:03 cameel

I'm trying to understand some of the test failures. I've accepted and pushed the gas changes when the new gas was lower than previous. However, there seem to be cases where the gas is higher, but I don't understand why. The simplest case is this: /test/libsolidity/semanticTests/abiEncodeDecode/abi_decode_simple_storage.sol

contract C {
    bytes data;

    function f(bytes memory _data) public returns (uint256, bytes memory) {
        data = _data;
        return abi.decode(data, (uint256, bytes));
    }
}

When I compare the assembly output with command line options (--via-ir --optimize --optimize-runs 2) I get this diff, which is fully expected and should reduce gas (diff of new vs old):

36c36
<       shr(0xc0, not(0x00))
---
>       sub(shl(0x40, 0x01), 0x01)
95c95
<       shr(0xc0, not(0x00))
---
>       sub(shl(0x40, 0x01), 0x01)
216c216
<       shr(0xc0, not(0x00))
---
>       sub(shl(0x40, 0x01), 0x01)
541c541
<       shr(0xc0, not(0x00))
---
>       sub(shl(0x40, 0x01), 0x01)
554c554
<       shr(0xc0, not(0x00))
---
>       sub(shl(0x40, 0x01), 0x01)
600c600
<     auxdata: 0xa264697066735822122096febbec567d55bd1ae8922a0aefa48fe6f302319ea990f739a29b6488e274ed64736f6c63782c302e382e33302d646576656c6f702e323032352e332e31322b636f6d6d69742e37393366633337302e6d6f64005d
---
>     auxdata: 0xa2646970667358221220760095869eecabd1c60e54b4c265518da61b9487bc75d680002076df84b1975664736f6c634300081c0033

Is it fair to assume auxdata is not part of the gas? Why is this showing more gas now? old: // gas irOptimized: 135499 new: // gas irOptimized: 135563

The only thing I can think of is somehow the old code was using constants because it's optimized for speed,not size. Is that possible?

Mar 12 '25 18:03 moh-eulith

The only thing I can think of is somehow the old code was using constants because it's optimized for speed,not size. Is that possible?

That doesn't seem like the answer either. Compiling for speed produces identical asm.

Mar 12 '25 18:03 moh-eulith

Is it fair to assume auxdata is not part of the gas?

auxdata is basically metadata and is part of the binary. It does affect gas at creation time because it has to be copied to memory along with the rest of the bytecode. We account for the code cost separately, but that's just the cost of storing it in calldata and does not include any operations that may be performed on it by the contract.

Mar 12 '25 19:03 cameel

The optimized tests run per default with runs set to 200, since that's the default compiler setting (it's arguable whether that's a good default, but it's been so historically) - with that on the case in question I currently see the masks as constants on develop - without having looked at it more closely my guess would be that the increase in runtime gas cost under that runs value pays off with a decrease in code size (which the test in this case doesn't display). The gas costs in the tests are meant to give a good estimation of the effect of changes of code, but since we don't generate them for the boundary runs settings, it can be fine if some of them increase if the reason is well understood.

Mar 12 '25 19:03 ekpyron

The optimized tests run per default with runs set to 200, since that's the default compiler setting (it's arguable whether that's a good default, but it's been so historically) - with that on the case in question I currently see the masks as constants on develop - without having looked at it more closely my guess would be that the increase in runtime gas cost under that runs value pays off with a decrease in code size (which the test in this case doesn't display). The gas costs in the tests are meant to give a good estimation of the effect of changes of code, but since we don't generate them for the boundary runs settings, it can be fine if some of them increase if the reason is well understood.

Yes, I can now see that locally now with --show-messages --show-metadata I've looked at the abi_decode_simple_storage test and decompiled the output before and after this change. There is just one change: a 64 bit mask was encoded as PUSH8 0xffffffffffffffff and is now

PUSH0     
NOT       
PUSH1     0xc0
SHR

For a low runs, this is clearly fewer bytes (9 bytes before, 5 bytes now). Runtime gas is more, but that's expected.

I've also verified that high runs (I used 2000000 -- a bit unclear on the exact thresholds), constants are present in the bytecode.

I haven't seen any failure in test function call results. I'll push the increased gas changes in a separate commit so you can have a look and let me know if it's acceptable.

Mar 12 '25 20:03 moh-eulith

The optimized tests run per default with runs set to 200,

The runs parameter is the critical piece of info here. I tested what happens to constants (specifically, 64bit mask) with the new representation and different run values.

runs	old	new
800	const	const
400	const	code
200	const	code
100	const	code
50	code	code

So somehow, the new code shifts the threshold between constant and code. ~~I've looked and I can't seem to find how the runs value is passed into and how it affects the optimizer (clearly, it does!). A little help is appreciated.~~ I'm fine with tweaking it to be closer to the old one, if it doesn't create more problems.

Mar 12 '25 21:03 moh-eulith

I've looked and I can't seem to find how the runs value is passed into and how it affects the optimizer (clearly, it does!).

never mind, found it. ConstantOptimizer.h, line 91. If I tweak it with a 3x the runs gas, we get the old behavior for a 64bit mask, but also constants that are around that size appear as literals. Results of doing that are here: https://github.com/moh-eulith/solidity/commit/f33c5df9a65bfc09c54126e65d8e47da5ce717a9

Let me know if you prefer one or the other.

Mar 12 '25 22:03 moh-eulith

After looking at the summary results, I prefer the PR as is (it doesn't have high outliers like the 3x tweak).

Results of this PR as is:

File name	IR optimized	Legacy optimized
`structs/conversion/recursive_storage_memory.sol`	1%
`array/copying/array_copy_cleanup_uint40.sol`	+0%	+0%
`externalContracts/snark.sol`	+0%	+0%
`array/copying/array_copy_different_packing.sol`	+0%	+0%
`array/copying/copy_internal_function_array_to_storage.sol`		+0%
`array/copying/copy_function_internal_storage_array.sol`	+0%	+0%
`array/copying/array_of_function_external_storage_to_storage_dynamic_different_mutability.sol`	+0%	+0%
`array/copying/array_of_function_external_storage_to_storage_dynamic.sol`	+0%	+0%
`externalContracts/base64.sol`	1%	-1%
`array/copying/function_type_array_to_storage.sol`	+0%	+0%
`abiEncodeDecode/abi_decode_simple_storage.sol`	+0%	+0%
`array/copying/array_copy_storage_storage_different_base.sol`	+0%	+0%
`array/arrays_complex_from_and_to_storage.sol`	+0%	+0%
`array/copying/nested_array_of_structs_calldata_to_storage.sol`	+0%
`abiEncoderV1/abi_decode_v2_storage.sol`	+0%	+0%
`array/copying/elements_of_nested_array_of_structs_calldata_to_storage.sol`	+0%
`structs/copy_substructures_to_mapping.sol`	+0%	+0%
`structs/copy_to_mapping.sol`	+0%	+0%
`storage/empty_nonempty_empty.sol`	+0%	+0%
`array/copying/array_of_structs_containing_arrays_calldata_to_storage.sol`	+0%
`array/copying/array_elements_to_mapping.sol`	+0%
`array/copying/calldata_array_to_mapping.sol`	+0%
`structs/calldata/calldata_struct_with_nested_array_to_storage.sol`	+0%	+0%
`array/copying/storage_memory_nested_struct.sol`	+0%
`abiEncoderV2/calldata_overlapped_dynamic_arrays.sol`	+0%	+0%
`various/skip_dynamic_types_for_structs.sol`	+0%	+0%
`types/mapping/copy_from_mapping_to_mapping.sol`	+0%
`structs/copy_from_mapping.sol`	+0%
`structs/copy_substructures_from_mapping.sol`	+0%
`array/copying/copy_byte_array_in_struct_to_storage.sol`	+0%
`various/destructuring_assignment.sol`	+0%	+0%
`array/copying/nested_dynamic_array_element_calldata_to_storage.sol`	+0%
`array/copying/array_to_mapping.sol`	+0%
`array/copying/array_of_struct_memory_to_storage.sol`	+0%
`structs/struct_containing_bytes_copy_and_delete.sol`	+0%	+0%
`array/copying/array_nested_calldata_to_storage.sol`	+0%
`structs/struct_memory_to_storage_function_ptr.sol`	+0%	-0%
`array/push/array_push_struct.sol`	+0%	+0%
`array/copying/arrays_from_and_to_storage.sol`		+0%
`various/many_subassemblies.sol`	+0%
`array/copying/array_storage_multi_items_per_slot.sol`	+0%	+0%
`constructor/no_callvalue_check.sol`	+0%
`immutable/multi_creation.sol`	+0%	-0%
`array/push/array_push_struct_from_calldata.sol`		+0%
`array/copying/copying_bytes_multiassign.sol`	+0%
`array/copying/array_of_struct_calldata_to_storage.sol`	+0%
`array/nested_calldata_storage2.sol`	+0%
`abiEncoderV2/storage_array_encoding.sol`	+0%
`array/copying/array_copy_target_simple_2.sol`	+0%	-0%
`structs/memory_structs_nested_load.sol`	+0%	+0%
`structs/structs.sol`	+0%	+0%
`array/copying/array_copy_storage_storage_dyn_dyn.sol`	+0%
`array/invalid_encoding_for_storage_byte_array.sol`	+0%
`saltedCreate/salted_create_with_value.sol`	+0%	-0%
`calldata/copy_from_calldata_removes_bytes_data.sol`	+0%
`various/staticcall_for_view_and_pure.sol`	+0%
`storageLayoutSpecifier/virtual_functions.sol`
`storageLayoutSpecifier/variable_cleanup_sstore.sol`
`storageLayoutSpecifier/variable_cleanup.sol`
`storageLayoutSpecifier/transient_state_variable_slot_offset.sol`
`storageLayoutSpecifier/storage_reference_library_function.sol`
`storageLayoutSpecifier/storage_reference_inheritance.sol`
`storageLayoutSpecifier/storage_reference_array.sol`
`storageLayoutSpecifier/state_variables_transient.sol`
`storageLayoutSpecifier/state_variable_struct.sol`
`storageLayoutSpecifier/state_variable_slot_offset.sol`
`storageLayoutSpecifier/state_variable_reference_types_slot_offset.sol`
`storageLayoutSpecifier/state_variable_mapping.sol`
`storageLayoutSpecifier/state_variable_enum.sol`
`storageLayoutSpecifier/state_variable_dynamic_array.sol`
`storageLayoutSpecifier/state_variable_constant_and_immutable.sol`
`storageLayoutSpecifier/state_variable_arithmetic_expression.sol`
`storageLayoutSpecifier/multiple_inheritance_state_var_slots.sol`
`storageLayoutSpecifier/multiple_inheritance.sol`
`storageLayoutSpecifier/mapping_storage_end.sol`
`storageLayoutSpecifier/last_allowed_storage_slot.sol`
`storageLayoutSpecifier/inline_assembly_direct_store.sol`
`storageLayoutSpecifier/inline_assembly_direct_load.sol`
`storageLayoutSpecifier/inheritance_state_variable_slot_offset.sol`
`storageLayoutSpecifier/inheritance_simple.sol`
`storageLayoutSpecifier/inheritance_from_same_base_state_var_slots.sol`
`storageLayoutSpecifier/inheritance_from_interface.sol`
`storageLayoutSpecifier/inheritance_from_abstract_contract.sol`
`storageLayoutSpecifier/getters.sol`
`storageLayoutSpecifier/function_from_base_contract.sol`
`storageLayoutSpecifier/dynamic_array_storage_end.sol`
`storageLayoutSpecifier/delete_transient_storage.sol`
`storageLayoutSpecifier/delete.sol`
`storageLayoutSpecifier/constructor.sol`
`storageLayoutSpecifier/base_slot_max_value.sol`
`various/different_call_type_transient.sol`	-0%	-0%
`array/delete/bytes_delete_element.sol`		-0%
`errors/small_error_optimization.sol`	-0%	-0%
`array/copying/array_copy_storage_storage_struct.sol`	-0%
`functionTypes/store_function.sol`		-0%
`libraries/internal_types_in_library.sol`		-0%
`abiEncoderV2/abi_encode_v2.sol`	-0%	-0%
`array/push/nested_bytes_push.sol`	-0%	-0%
`array/create_memory_array.sol`	-0%	-0%
`array/reusing_memory.sol`	-0%	-0%
`array/fixed_arrays_as_return_type.sol`	-0%	-0%
`structs/struct_delete_storage_with_array.sol`	-0%	-0%
`viaYul/copy_struct_invalid_ir_bug.sol`	-0%	-0%
`array/copying/array_copy_target_simple.sol`	-0%	-0%
`inheritance/constructor_with_params_diamond_inheritance.sol`	-0%
`inheritance/constructor_with_params_inheritance.sol`	-0%
`array/copying/array_copy_storage_storage_different_base_nested.sol`	-0%	-0%
`array/copying/nested_array_of_structs_memory_to_storage.sol`	-0%
`inheritance/constructor_with_params.sol`	-0%
`smoke/constructor.sol`	-0%
`array/push/array_push_nested_from_calldata.sol`	-0%	-0%
`immutable/use_scratch.sol`	-0%
`array/copying/elements_of_nested_array_of_structs_memory_to_storage.sol`	-0%
`storage/packed_storage_structs_bytes.sol`	-0%	-0%
`array/copying/memory_dyn_2d_bytes_to_storage.sol`	-0%	-0%
`array/copying/array_copy_target_leftover.sol`	-0%	-0%
`constructor/constructor_static_array_argument.sol`	-0%	-0%
`array/copying/storage_memory_nested_from_pointer.sol`	-0%	-0%
`array/copying/storage_memory_nested.sol`	-0%	-0%
`array/constant_var_as_array_length.sol`	-0%	-0%
`operators/userDefined/operator_making_view_external_call.sol`		-0%
`operators/userDefined/operator_making_pure_external_call.sol`		-0%
`userDefinedValueType/calldata.sol`	-0%	-0%
`immutable/immutable_tag_too_large_bug.sol`	-0%
`constructor/bytes_in_constructors_unpacker.sol`	-0%	-0%
`various/create_calldata.sol`	-0%	-0%
`array/pop/byte_array_pop_long_storage_empty.sol`	-0%	-0%
`array/byte_array_transitional_2.sol`		-0%
`array/push/byte_array_push_transition.sol`		-0%
`various/address_code.sol`	-0%	-0%
`externalContracts/prbmath_signed.sol`	-0%	-0%
`externalContracts/strings.sol`	-0%	-0%
`array/copying/bytes_storage_to_storage.sol`	-0%	-1%
`events/event_emit_from_other_contract.sol`	-0%	-0%
`functionCall/creation_function_call_with_args.sol`	-0%	-1%
`functionCall/creation_function_call_with_salt.sol`	-0%	-1%
`externalContracts/deposit_contract.sol`	-0%	-1%
`various/senders_balance.sol`	-0%	-1%
`constructor/constructor_arguments_external.sol`	-1%	-1%
`functionCall/failed_create.sol`	-2%
`cleanup/byte_array_to_storage_cleanup.sol`	-1%	-1%
`constructor/bytes_in_constructors_packer.sol`	-1%	-1%
`array/fixed_arrays_in_constructors.sol`	-1%	-1%
`externalContracts/ramanujan_pi.sol`	-2%	-0%
`abiEncoderV2/calldata_array.sol`	-2%
`inheritance/inherited_function_calldata_memory_interface.sol`		-2%
`functionCall/gas_and_value_basic.sol`	-1%	-1%
`abiEncoderV2/abi_encode_v2_in_function_inherited_in_v1_contract.sol`	-2%	-0%
`constructor/arrays_in_constructors.sol`	-1%	-1%
`functionCall/external_call_to_nonexisting_debugstrings.sol`	-2%	-1%
`userDefinedValueType/erc20.sol`	-1%	-2%
`various/erc20.sol`	-1%	-2%
`inheritance/value_for_constructor.sol`	-2%	-2%
`abiEncoderV2/abi_encode_calldata_slice.sol`	-2%	-2%
`abiEncoderV1/abi_encode_calldata_slice.sol`	-2%	-2%
`array/function_array_cross_calls.sol`	-2%	-2%
`various/selfdestruct_post_cancun_multiple_beneficiaries.sol`	-2%	-2%
`inlineAssembly/transient_storage_selfdestruct.sol`	-2%	-2%
`various/selfdestruct_post_cancun_redeploy.sol`	-2%	-3%
`functionCall/external_call_to_nonexisting.sol`	-3%	-3%
`various/selfdestruct_post_cancun.sol`	-4%	-3%
`externalContracts/FixedFeeRegistrar.sol`	-5%	-4%

Results of 3x tweak:

File name	IR optimized	Legacy optimized
`externalContracts/prbmath_unsigned.sol`	5%	6%
`externalContracts/prbmath_signed.sol`	5%	5%
`constructor/constructor_arguments_external.sol`	5%	5%
`functionCall/external_call_to_nonexisting_debugstrings.sol`	+0%	4%
`events/event_emit_from_other_contract.sol`	2%	2%
`cleanup/byte_array_to_storage_cleanup.sol`	2%
`constructor/bytes_in_constructors_packer.sol`	-0%	2%
`immutable/immutable_tag_too_large_bug.sol`	1%
`array/fixed_arrays_in_constructors.sol`	2%	-1%
`various/senders_balance.sol`	2%	-1%
`functionCall/creation_function_call_with_args.sol`	1%	-1%
`functionCall/creation_function_call_with_salt.sol`	1%	-1%
`inheritance/value_for_constructor.sol`	1%	+0%
`array/function_array_cross_calls.sol`	1%	-1%
`array/copying/array_of_function_external_storage_to_storage_dynamic_different_mutability.sol`	+0%	+0%
`array/copying/array_of_function_external_storage_to_storage_dynamic.sol`	+0%	+0%
`array/copying/elements_of_nested_array_of_structs_calldata_to_storage.sol`	+0%
`array/copying/nested_array_of_structs_calldata_to_storage.sol`	+0%
`array/copying/array_elements_to_mapping.sol`	+0%
`array/copying/array_copy_storage_storage_different_base.sol`	+0%
`structs/copy_to_mapping.sol`	+0%
`externalContracts/ramanujan_pi.sol`	-0%	+0%
`various/many_subassemblies.sol`	+0%
`structs/memory_structs_nested_load.sol`	+0%	+0%
`structs/structs.sol`	+0%	+0%
`structs/copy_substructures_to_mapping.sol`	+0%	-0%
`array/copying/copy_byte_array_in_struct_to_storage.sol`	+0%	-0%
`storageLayoutSpecifier/virtual_functions.sol`
`storageLayoutSpecifier/variable_cleanup_sstore.sol`
`storageLayoutSpecifier/variable_cleanup.sol`
`storageLayoutSpecifier/transient_state_variable_slot_offset.sol`
`storageLayoutSpecifier/storage_reference_library_function.sol`
`storageLayoutSpecifier/storage_reference_inheritance.sol`
`storageLayoutSpecifier/storage_reference_array.sol`
`storageLayoutSpecifier/state_variables_transient.sol`
`storageLayoutSpecifier/state_variable_struct.sol`
`storageLayoutSpecifier/state_variable_slot_offset.sol`
`storageLayoutSpecifier/state_variable_reference_types_slot_offset.sol`
`storageLayoutSpecifier/state_variable_mapping.sol`
`storageLayoutSpecifier/state_variable_enum.sol`
`storageLayoutSpecifier/state_variable_dynamic_array.sol`
`storageLayoutSpecifier/state_variable_constant_and_immutable.sol`
`storageLayoutSpecifier/state_variable_arithmetic_expression.sol`
`storageLayoutSpecifier/multiple_inheritance_state_var_slots.sol`
`storageLayoutSpecifier/multiple_inheritance.sol`
`storageLayoutSpecifier/mapping_storage_end.sol`
`storageLayoutSpecifier/last_allowed_storage_slot.sol`
`storageLayoutSpecifier/inline_assembly_direct_store.sol`
`storageLayoutSpecifier/inline_assembly_direct_load.sol`
`storageLayoutSpecifier/inheritance_state_variable_slot_offset.sol`
`storageLayoutSpecifier/inheritance_simple.sol`
`storageLayoutSpecifier/inheritance_from_same_base_state_var_slots.sol`
`storageLayoutSpecifier/inheritance_from_interface.sol`
`storageLayoutSpecifier/inheritance_from_abstract_contract.sol`
`storageLayoutSpecifier/getters.sol`
`storageLayoutSpecifier/function_from_base_contract.sol`
`storageLayoutSpecifier/dynamic_array_storage_end.sol`
`storageLayoutSpecifier/delete_transient_storage.sol`
`storageLayoutSpecifier/delete.sol`
`storageLayoutSpecifier/constructor.sol`
`storageLayoutSpecifier/base_slot_max_value.sol`
`inlineAssembly/tstore_hidden_staticcall.sol`		-0%
`inlineAssembly/transient_storage_low_level_calls.sol`	-0%
`various/different_call_type_transient.sol`	-0%	-0%
`array/push/array_push_struct.sol`	-0%	+0%
`libraries/internal_types_in_library.sol`		-0%
`array/copying/nested_array_of_structs_storage_to_storage.sol`	-0%
`array/copying/array_of_structs_containing_arrays_calldata_to_storage.sol`	-0%
`array/nested_calldata_storage2.sol`	-0%
`structs/copy_struct_array_from_storage.sol`	-0%
`array/push/array_push_struct_from_calldata.sol`	-0%
`types/mapping/copy_from_mapping_to_mapping.sol`	-0%
`array/copying/array_copy_including_array.sol`	-0%
`structs/calldata/calldata_struct_with_nested_array_to_storage.sol`	-0%	-0%
`array/copying/storage_memory_packed_dyn.sol`	-0%
`errors/small_error_optimization.sol`	-0%	-0%
`array/copying/array_copy_nested_array.sol`	-0%
`array/copying/nested_array_element_storage_to_storage.sol`	-0%
`array/copying/storage_memory_nested_bytes.sol`	-0%
`array/copying/calldata_array_to_mapping.sol`	-0%
`events/event_dynamic_nested_array_storage_v2.sol`	-0%
`various/skip_dynamic_types_for_structs.sol`	-0%	-0%
`array/copying/array_storage_multi_items_per_slot.sol`	-0%	-0%
`abiEncoderV2/abi_encode_v2_in_function_inherited_in_v1_contract.sol`	-0%	-0%
`array/arrays_complex_from_and_to_storage.sol`	-0%
`array/push/array_push.sol`	-0%
`array/copying/function_type_array_to_storage.sol`	+0%	-0%
`array/delete/bytes_delete_element.sol`	-0%	-0%
`array/copying/copy_function_internal_storage_array.sol`	-0%
`immutable/multi_creation.sol`	-0%	-0%
`various/destructuring_assignment.sol`	-0%	-0%
`saltedCreate/salted_create_with_value.sol`	-0%	-0%
`structs/struct_containing_bytes_copy_and_delete.sol`	-0%
`inheritance/inherited_function_calldata_memory_interface.sol`		-0%
`structs/struct_memory_to_storage_function_ptr.sol`		-0%
`array/create_memory_array.sol`	-0%	-0%
`array/pop/array_pop_array_transition.sol`	-0%
`storage/empty_nonempty_empty.sol`	-0%
`array/bytes_length_member.sol`	-0%
`array/copying/array_nested_calldata_to_storage.sol`	-0%
`abiEncoderV1/abi_decode_v2_storage.sol`	-0%
`array/push/nested_bytes_push.sol`	-0%	-0%
`array/copying/array_copy_storage_storage_dyn_dyn.sol`	-0%
`array/copying/array_copy_storage_storage_struct.sol`	-0%
`abiEncoderV2/storage_array_encoding.sol`	-0%
`array/fixed_arrays_as_return_type.sol`	-0%	-0%
`array/reusing_memory.sol`	-0%	-0%
`array/copying/array_of_struct_memory_to_storage.sol`	-0%
`abiEncoderV2/abi_encode_v2.sol`	-0%	-0%
`inheritance/constructor_with_params_diamond_inheritance.sol`	-0%
`abiEncodeDecode/abi_decode_simple_storage.sol`	-0%
`array/copying/array_nested_memory_to_storage.sol`	-0%
`array/copying/array_copy_target_simple.sol`	-0%	-0%
`functionTypes/store_function.sol`		-0%
`inheritance/constructor_with_params_inheritance.sol`	-0%
`abiEncoderV2/calldata_overlapped_dynamic_arrays.sol`	-0%
`array/copying/nested_dynamic_array_element_calldata_to_storage.sol`	-0%
`array/dynamic_multi_array_cleanup.sol`	-0%
`array/copying/array_copy_cleanup_uint40.sol`	-0%	+0%
`array/copying/array_copy_target_simple_2.sol`		-0%
`inheritance/constructor_with_params.sol`	-0%
`smoke/constructor.sol`	-0%
`immutable/use_scratch.sol`	-0%
`array/copying/elements_of_nested_array_of_structs_memory_to_storage.sol`	-0%
`array/copying/array_of_structs_containing_arrays_memory_to_storage.sol`	-0%
`array/copying/copy_byte_array_to_storage.sol`	-0%
`array/copying/array_copy_storage_storage_different_base_nested.sol`	-0%	-0%
`externalContracts/snark.sol`	-0%	-0%
`array/push/array_push_nested_from_calldata.sol`	-0%	-0%
`array/copying/copying_bytes_multiassign.sol`	-0%
`array/copying/nested_array_of_structs_memory_to_storage.sol`	-0%
`storage/packed_storage_structs_bytes.sol`	-0%	-0%
`array/copying/array_to_mapping.sol`	-0%	-0%
`array/copying/array_of_struct_calldata_to_storage.sol`	-0%
`array/copying/calldata_array_dynamic_to_storage.sol`	-0%
`constructor/constructor_static_array_argument.sol`	-0%	-0%
`viaYul/copy_struct_invalid_ir_bug.sol`	-0%	-0%
`array/constant_var_as_array_length.sol`	-0%	-0%
`array/copying/storage_memory_nested_from_pointer.sol`	-0%	-0%
`array/copying/storage_memory_nested.sol`	-0%	-0%
`array/pop/byte_array_pop_masking_long.sol`	-0%
`operators/userDefined/operator_making_view_external_call.sol`		-0%
`operators/userDefined/operator_making_pure_external_call.sol`		-0%
`array/copying/array_copy_clear_storage.sol`	-0%
`structs/struct_delete_storage_with_array.sol`	-0%	-0%
`array/copying/array_copy_different_packing.sol`	-0%	-0%
`constructor/bytes_in_constructors_unpacker.sol`	-0%	-0%
`various/create_calldata.sol`	-0%	-0%
`array/copying/memory_dyn_2d_bytes_to_storage.sol`	-0%	-0%
`array/byte_array_transitional_2.sol`		-0%
`userDefinedValueType/calldata.sol`	-0%	-0%
`array/push/push_no_args_2d.sol`	-0%
`array/copying/array_copy_target_leftover.sol`	-0%	-0%
`externalContracts/strings.sol`	-0%	-0%
`various/address_code.sol`	-0%	-0%
`array/pop/byte_array_pop_long_storage_empty_garbage_ref.sol`	-0%
`externalContracts/deposit_contract.sol`	-0%	-0%
`array/dynamic_arrays_in_storage.sol`	-1%
`array/push/byte_array_push_transition.sol`		-1%
`array/pop/byte_array_pop_long_storage_empty.sol`	-1%	-0%
`constructor/arrays_in_constructors.sol`	-0%	-1%
`various/selfdestruct_post_cancun_redeploy.sol`	-0%	-2%
`array/copying/bytes_storage_to_storage.sol`	-1%	-1%
`abiEncoderV2/calldata_array.sol`	-2%
`various/selfdestruct_post_cancun_multiple_beneficiaries.sol`	+0%	-2%
`functionCall/gas_and_value_basic.sol`	-1%	-1%
`inlineAssembly/transient_storage_selfdestruct.sol`	+0%	-2%
`functionCall/external_call_to_nonexisting.sol`	-2%	-2%
`externalContracts/base64.sol`	-2%	-1%
`userDefinedValueType/erc20.sol`	-1%	-2%
`various/erc20.sol`	-1%	-2%
`various/selfdestruct_post_cancun.sol`	-2%	-3%
`externalContracts/FixedFeeRegistrar.sol`	-4%	-3%
`abiEncoderV2/abi_encode_calldata_slice.sol`	-2%	-6%
`abiEncoderV1/abi_encode_calldata_slice.sol`	-2%	-6%

Mar 12 '25 22:03 moh-eulith

The only failing tests are for the new eof/osaka changes. Constant optimizer is explicitly disabled:

	// TODO: investigate for EOF
	if (_settings.runConstantOptimiser && !m_eofVersion.has_value())

Not sure what the right way to handle the test cases are. Can I supply a different outcome? Can I exclude the test for EOF only?

Mar 13 '25 00:03 moh-eulith

My pedestrian solution for EOF and constant optimizer. Happy to include in this PR or another, or just ignore.

https://github.com/moh-eulith/solidity/commit/5805f6da4820929886bf2c9a52bec45e703449d1

Mar 13 '25 02:03 moh-eulith

I decided messing with the default behavior was not appropriate. Changed the thresholds for the default runs of 200 so that the size optimization doesn't kick in for simple constants. The change is now strictly better (at 200), and will favor size reduction below 200.

Mar 14 '25 16:03 moh-eulith

Not sure what the right way to handle the test cases are. Can I supply a different outcome? Can I exclude the test for EOF only?

In many test types you can use the bytecodeFormat setting to limit the test to non-EOF:

// ====
// bytecodeFormat: legacy

Normally you'd create a separate version with different expectations for EOF, but since the constant optimizer is disabled there, it's fine to just leave a TODO comment as a reminder to do that later.

My pedestrian solution for EOF and constant optimizer. Happy to include in this PR or another, or just ignore.

https://github.com/moh-eulith/solidity/commit/5805f6da4820929886bf2c9a52bec45e703449d1

If that works then perhaps the constant optimizer does not even need much changes for EOF, but I'd still rather not enable it just as an afterthought in this PR. Even if it compiles and passes current tests, it does not necessarily mean it's not broken in some subtle way. At this point we do have working semantic tests so there's a high chance it's not, but our coverage is not always perfect, especially for things that depend on the runs parameter (most tests just run with 200). https://github.com/ethereum/solidity/pull/15935#discussion_r1996488454 is a good example. This particular problem won't affect it yet because we're not using any EOF-only instructions there, but there could be similar ones lurking in there so it needs to be given proper attention.

Mar 15 '25 01:03 cameel

Regarding costs, the ones from test cases may not always be representative, because they are rather artificial. We should also check how it affects real projects (i.e. our external tests). You can do that by getting the summarized-benchmarks.json artifact from the c_ext_benchmarks job from your PR and from the base branch and comparing them using benchmark_diff.py. Check --help. It can produce a markdown table that you can just paste into a comment.

Mar 15 '25 01:03 cameel

I'd still rather not enable it just as an afterthought in this PR.

Agreed. The constant optimizer is relatively straight forward to reason about, as it's completely self contained. For the record, here are the instructions used in the 3 different paths:

constant: PUSHX

compute: PUSHX, EXP, NOT, MUL, ADD, SUB, SHL, SHR

copy: CODECOPY, MLOAD, SWAP1, MSTORE, DUP1, DUP2, DUP4, SWAP2

My code essentially disables the third branch. Just leaving here as a note for the next person who needs to deal with it.

Mar 15 '25 02:03 moh-eulith

summarized-benchmarks.json artifact from the c_ext_benchmarks job from your PR and from the base branch

Thanks for the pointer. This PR as originally written (no 200 heuristic) vs. develop:

`ir-optimize-evm+yul`

project	bytecode_size	deployment_gas	method_gas
brink	`-2.09% ✅`
colony	`-2.32% ✅`
elementfi	`-1.47% ✅`
ens	`-2.11% ✅`	`-2.5% ✅`	`-0.06% ✅`
euler	`-2.9% ✅`	`-2.93% ✅`	`-0.34% ✅`
gnosis
gp2	`-1.89% ✅`
pool-together	`-2.29% ✅`
uniswap	`-2.77% ✅`
yield_liquidator	`-3.42% ✅`	`-3.53% ✅`	`-0.05% ✅`
zeppelin	`-2.54% ✅`

This PR as is now (with 200 heuristic) vs develop:

`ir-optimize-evm+yul`

project	bytecode_size	deployment_gas	method_gas
brink	`-2.09% ✅`
colony	`-1.91% ✅`
elementfi	`-1% ✅`
ens	`-1.21% ✅`	`-1.75% ✅`	`-0.05% ✅`
euler	`-1.84% ✅`	`-1.82% ✅`	`-0.25% ✅`
gnosis
gp2	`-1.25% ✅`
pool-together	`-1.62% ✅`
uniswap	`-1.17% ✅`
yield_liquidator	`-2.13% ✅`	`-2.07% ✅`	`-0.07% ✅`
zeppelin	`-1.56% ✅`

I'm not really sure what method_gas is being measured, so it's hard to opine on what that means. (Edit: now with fancy tables)

Mar 15 '25 03:03 moh-eulith

I'm not really sure what method_gas is being measured, so it's hard to opine on what that means.

This is the total execution cost measured by running project's test suite. Hard to say how representative it is, but at least it comes from actually executing most of project's code.

We should actually be getting method_gas for more projects. I've seen some failures in parsing the coverage report in CI (which is where the values come from). We should look into that. It does not make CI fail, because we intentionally ignore errors in getting benchmarks.

Also, just FYI, what you pasted are the tables in format that's better suited for the terminal (which is why it's the default), but there's a flag that will give you more fancy markdown as well :)

Mar 15 '25 05:03 cameel

We should actually be getting method_gas for more projects.

I double checked and it's null in the json for develop branch. Do you know which ones? I managed to run zeppelin and it looks like it never reports any gas metrics. Some are hard to run locally (need older version of foundry, etc).

Mar 15 '25 05:03 moh-eulith

The build is green (I tried to fix the spurious failures).

Still todo: test cases using the new test framework (no rush, just keeping track of what's left).

anything else?

Mar 17 '25 17:03 moh-eulith

I added a semantic test for masks. Most masks are generated by the compiler internally, so it made sense to me to test those. (of the categories under /test semantic felt like the best fit; let me know if it fits better elsewhere)

Mar 25 '25 17:03 moh-eulith

A semantic test is nice, but it would be good to also have some unit tests for the optimization step itself - since there's no particular rush, it's probably easiest to wait for @cameel to build a new nicer framework for such tests (see https://github.com/ethereum/solidity/pull/15935#issuecomment-2718750702) which we could use for other purposes anyways (@cameel is currently off, though, but I hope he'll be back next week)

Apr 02 '25 14:04 ekpyron

A semantic test is nice, but it would be good to also have some unit tests for the optimization step itself - since there's no particular rush, it's probably easiest to wait for @cameel to build a new nicer framework for such tests (see #15935 (comment)) which we could use for other purposes anyways (@cameel is currently off, though, but I hope he'll be back next week)

Agreed.

Apr 02 '25 14:04 moh-eulith

@ekpyron I noticed there is an unused (?) test framework for the constant optimizer via solfuzzer. I added a human readable input option (--hex) and tested various masks and constants. It runs the constant optimizer directly with ten different optimizer runs settings. Adding more constants is as easy as adding a line in the constants.txt file.

Still happy to wait for the new testing framework, unless you think this is sufficient.

~~P.S. I have no idea what the osx build failure is about. this branch is up to date with develop. help?~~

Apr 02 '25 19:04 moh-eulith

Just a heads up, we still do want to have the assembly tests here. We generally should have this kind of coverage for the optimizer and it will be useful beyond this PR alone. There's been a lot of distractions with other topics recently, but I'm back to working on this, so it'll be ready soon.

Apr 10 '25 21:04 cameel

I reorganized the commits. The first commit adds tests with no code changes using @cameel 's new testing framework. (I like the flexibility that allows better coverage for different optimization levels.)

Three tests show how the optimizer works at 0, 200 and 2M runs.

The second commit is the code. Looking at the tests changes gives a pretty good indication of how the form changes with runs and how it's more optimal than previously.

Apr 24 '25 16:04 moh-eulith

Could I please get a review in the next few days? I have an upcoming trip and may not be able to give this the attention it may require. Thanks.

May 16 '25 19:05 moh-eulith