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.

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

Function deduplication #5167

Merged
merged 8 commits into from
Oct 4, 2023
Merged

Function deduplication #5167

merged 8 commits into from
Oct 4, 2023

Conversation

vaivaswatha
Copy link
Contributor

This optimisation pass looks for identical functions and eliminates all but one, replacing calls appropriately.

Ideally this must be run before any other pass, to reduce compile times for all the remaining passes. However, when run before inlining, it impacts some inlining decisions. For example, functions which previously had only a single caller now may have multiple callers, thus preventing them from being inlined. While theoretically that could be good, numbers indicated otherwise. The inliner needs to be tuned before we move dedup ahead of it.

Also, the PR has a minor edit to the order in which inlining is performed on inlining candidates. We now follow lexical order rather than relying on an arbitrary order set by FxHashMap. This was necessary so that equal functions remain equal after inlining. Changing the order of inlining resulted in differences in the order and names of local variables, rendering functions
that were equal before inlining to not be anymore.

Closes #5166

Code size impact on sdk-harness contracts/scripts:

test before after reduction
methods_abi 28 28 0
vec_in_abi 7892 7892 0
type_aliases 172 172 0
tx_predicate 36 36 0
tx_output_predicate 828 828 0
tx_contract 7480 7480 0
token_ops 14332 8512 40.608428691041
test_script_bytes 1140 1140 0
test_script 1620 1620 0
test_contract 6332 5140 18.8250157927985
svec_u8 35800 23640 33.9664804469274
svec_u64 34872 23640 32.2092222986924
svec_u32 35800 23640 33.9664804469274
svec_u16 35800 23640 33.9664804469274
svec_tuple 37192 25048 32.6521832652183
svec_struct 37048 24904 32.7790973871734
svec_str 37040 24896 32.7861771058315
svec_enum 37048 24904 32.7790973871734
svec_bool 35800 23640 33.9664804469274
svec_b256 37040 24896 32.7861771058315
svec_array 37768 25624 32.1542046176657
superabi_supertrait 124 124 0
superabi 124 124 0
storage_vec_to_vec 13308 8720 34.4755034565675
storage_vec_nested 51948 47968 7.66150766150766
storage_string 4620 3436 25.6277056277056
storage_map_nested 74848 57828 22.739418554938
storage_map 120600 40824 66.1492537313433
storage_init 19296 17764 7.93946932006634
storage_bytes 7140 5216 26.9467787114846
storage_access 38388 30712 19.995832030843
storage 16152 12328 23.6750866765726
script_data 392 392 0
script_bytecode 624 624 0
result_in_abi 516 516 0
registers 540 540 0
predicatepredicate_data_struct 388 388 0
predicatepredicate_data_simple 364 364 0
pow 228 228 0
parsing_logs_test_abi 28 28 0
parsing_logs 1244 1244 0
option_in_abi 516 516 0
option_field_order 2672 1804 32.4850299401198
methods_contract 4224 3984 5.68181818181818
messages 876 876 0
logging 400 400 0
hashing 18020 16836 6.57047724750278
generics_in_abi 2584 1732 32.9721362229102
evm_ec_recover 2088 2088 0
evm_test_abi 28 28 0
evm 224 224 0
ec_recover_and_match_predicate 4924 4924 0
ec_recover 2056 2048 0.389105058365759
contract_bytecode 168 168 0
context_testing_abi 28 28 0
context_caller_contract 8200 2568 68.6829268292683
context 404 404 0
configurables_in_script 252 252 0
configurables_in_contract 284 284 0
call_frames_test_abi 28 28 0
call_frames 1196 1196 0
block_test_abi 28 28 0
block 484 484 0
balance_contract 84 84 0
auth_testing_abi 28 28 0
auth_testing_contract 3012 2900 3.71845949535193
auth_caller_script 264 264 0
auth_caller_contract 176 176 0
abi_impl_methods_callable 116 116 0

@vaivaswatha vaivaswatha requested a review from a team October 3, 2023 11:43
@vaivaswatha vaivaswatha self-assigned this Oct 3, 2023
@vaivaswatha
Copy link
Contributor Author

There's a unit test ICE'ing. Changing this to Draft.

@vaivaswatha vaivaswatha marked this pull request as draft October 3, 2023 11:51
vaivaswatha and others added 4 commits October 3, 2023 17:28
AsmBlocks are part of an Instruction. When an Instruction
is cloned, they must be too. But if they're in the Arena,
when an instruction is cloned, we end up with the same AsmBlock
being part of two instructions. If a transformation modifies
one of it, the other gets modified too, which is incorrect.
@vaivaswatha
Copy link
Contributor Author

This PR, now, also modifies the IR infrstructure to not store AsmBlock in the arena. Details are in this commit message.

@vaivaswatha vaivaswatha marked this pull request as ready for review October 4, 2023 09:33
@vaivaswatha vaivaswatha enabled auto-merge (squash) October 4, 2023 15:34
@IGI-111 IGI-111 requested a review from a team October 4, 2023 16:57
@vaivaswatha vaivaswatha merged commit b0447a1 into master Oct 4, 2023
31 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add deduplication pass
3 participants