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

Advanced: Basic block metadata placement refactoring #457

Merged
merged 2 commits into from
Apr 12, 2022

Conversation

gumb0
Copy link
Member

@gumb0 gumb0 commented Apr 6, 2022

Resolves #445

@gumb0 gumb0 force-pushed the block-metadata-refactor branch 3 times, most recently from fb32239 to 8036a6b Compare April 7, 2022 11:00
@codecov
Copy link

codecov bot commented Apr 7, 2022

Codecov Report

Merging #457 (ef7b05e) into master (7846cb0) will increase coverage by 0.00%.
The diff coverage is 100.00%.

❗ Current head ef7b05e differs from pull request most recent head 1c26b64. Consider uploading reports for the commit 1c26b64 to get more accurate results

@@           Coverage Diff           @@
##           master     #457   +/-   ##
=======================================
  Coverage   99.58%   99.58%           
=======================================
  Files          37       37           
  Lines        4539     4543    +4     
=======================================
+ Hits         4520     4524    +4     
  Misses         19       19           
Flag Coverage Δ
consensus 79.10% <100.00%> (+0.35%) ⬆️
unittests 99.62% <100.00%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
lib/evmone/advanced_analysis.cpp 100.00% <100.00%> (ø)
lib/evmone/advanced_instructions.cpp 100.00% <100.00%> (ø)
test/unittests/analysis_test.cpp 100.00% <100.00%> (ø)

@gumb0 gumb0 force-pushed the block-metadata-refactor branch 5 times, most recently from 4c25348 to 20486e9 Compare April 7, 2022 12:29
@gumb0
Copy link
Member Author

gumb0 commented Apr 7, 2022

@chfast This currently works by keeping only single OPX_BEGINBLOCK as a first instruction to attach first block metadata (all other blocks' metadata attached only to JUMPDEST and JUMPI)

Do you think it's worth to try to get rid of this first OPX_BEGINBLOCK here by putting first block metadata to AdvancedCodeAnalysis perahaps and changing advanced::execute to start with a special case for first block initialization?

@chfast
Copy link
Member

chfast commented Apr 7, 2022

Do you think it's worth to try to get rid of this first OPX_BEGINBLOCK here by putting first block metadata to AdvancedCodeAnalysis perahaps and changing advanced::execute to start with a special case for first block initialization?

This is fine as a start, but at some point we have to make sure the OPX_BEGINBLOCK is always added (even if the first instruction is JUMPDEST) to have simple jump target translation (pc + 1).

@gumb0
Copy link
Member Author

gumb0 commented Apr 7, 2022

This is fine as a start, but at some point we have to make sure the OPX_BEGINBLOCK is always added (even if the first instruction is JUMPDEST) to have simple jump target translation (pc + 1).

It is always added here now.

@chfast chfast force-pushed the block-metadata-refactor branch from 20486e9 to 603327d Compare April 12, 2022 08:31
Copy link
Member

@chfast chfast left a comment

Choose a reason for hiding this comment

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

The max_instrs_size can be increased to code.size() + 2 (additional BEGINBLOCK and STOP). And the FIXME assert can be enabled in the analysis end.

@@ -234,6 +235,7 @@ constexpr std::array<instruction_exec_fn, 256> instruction_implementations = [](
table[OP_PC] = op_pc;
table[OP_GAS] = op_gas;
table[OPX_BEGINBLOCK] = opx_beginblock;
table[OP_JUMPDEST] = opx_beginblock;
Copy link
Member

Choose a reason for hiding this comment

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

The OPX_BEGINBLOCK is alias for OP_JUMPDEST :)

@gumb0 gumb0 force-pushed the block-metadata-refactor branch from 603327d to f7edc0d Compare April 12, 2022 11:03
@chfast chfast force-pushed the block-metadata-refactor branch from f7edc0d to b5b09cc Compare April 12, 2022 12:06
@chfast
Copy link
Member

chfast commented Apr 12, 2022

Analysis benchmarks

advanced/analyse/main/blake2b_huff_mean                                     -0.0665         -0.0665            53            49            53            49
advanced/analyse/main/blake2b_shifts_mean                                   -0.0041         -0.0041            23            23            23            23
advanced/analyse/main/sha1_divs_mean                                        -0.0533         -0.0533             5             5             5             5
advanced/analyse/main/sha1_shifts_mean                                      -0.0467         -0.0467             5             5             5             5
advanced/analyse/main/weierstrudel_mean                                     -0.1020         -0.1020            66            59            66            59
advanced/analyse/micro/beginsub_push1s_0xffff_mean                          -0.5907         -0.5907           206            84           206            84
advanced/analyse/micro/beginsubs_0xffff_mean                                -0.9386         -0.9386           366            22           366            22
advanced/analyse/micro/jump_around_mean                                     +0.1005         +0.1005            72            79            72            79
advanced/analyse/micro/jumpdests_0xffff_mean                                +0.4966         +0.4966           375           561           375           561
advanced/analyse/micro/loop_with_many_jumpdests_mean                        +0.3246         +0.3246           147           195           147           195
advanced/analyse/micro/memory_grow_mload_mean                               -0.0677         -0.0677            97            91            97            91
advanced/analyse/micro/memory_grow_mstore_mean                              -0.0706         -0.0706            96            89            96            89
advanced/analyse/micro/push1s_0xffff_mean                                   -0.5390         -0.5390           194            90           194            90
advanced/analyse/micro/push32s_0xffff_mean                                  -0.8630         -0.8630            40             6            40             6
advanced/analyse/micro/signextend_mean                                      -0.0643         -0.0643           113           106           113           106
advanced/analyse/micro/zeros_0xffff_mean                                    -0.9552         -0.9552           501            22           501            22

@chfast
Copy link
Member

chfast commented Apr 12, 2022

Execution benchmarks

advanced/execute/main/blake2b_huff/empty_mean                               +0.0067         +0.0067            12            12            12            12
advanced/execute/main/blake2b_huff/2805nulls_mean                           +0.0005         +0.0005           261           261           261           261
advanced/execute/main/blake2b_huff/5610nulls_mean                           +0.0037         +0.0037           508           510           508           510
advanced/execute/main/blake2b_huff/8415nulls_mean                           -0.0015         -0.0015           747           746           747           746
advanced/execute/main/blake2b_huff/65536nulls_mean                          -0.0008         -0.0008          5812          5808          5812          5808
advanced/execute/main/blake2b_shifts/2805nulls_mean                         -0.0202         -0.0202          3428          3359          3428          3359
advanced/execute/main/blake2b_shifts/5610nulls_mean                         -0.0203         -0.0203          6842          6703          6842          6703
advanced/execute/main/blake2b_shifts/8415nulls_mean                         -0.0198         -0.0198         10256         10052         10256         10052
advanced/execute/main/blake2b_shifts/65536nulls_mean                        -0.0166         -0.0166         79718         78393         79718         78394
advanced/execute/main/sha1_divs/empty_mean                                  +0.0047         +0.0047            51            51            51            51
advanced/execute/main/sha1_divs/1351_mean                                   +0.0018         +0.0018          1052          1054          1052          1054
advanced/execute/main/sha1_divs/2737_mean                                   +0.0013         +0.0013          2053          2055          2053          2055
advanced/execute/main/sha1_divs/5311_mean                                   +0.0007         +0.0007          4008          4011          4008          4011
advanced/execute/main/sha1_divs/65536_mean                                  +0.0017         +0.0017         48872         48957         48872         48957
advanced/execute/main/sha1_shifts/empty_mean                                +0.0058         +0.0058            29            29            29            29
advanced/execute/main/sha1_shifts/1351_mean                                 +0.0026         +0.0026           616           618           616           618
advanced/execute/main/sha1_shifts/2737_mean                                 +0.0036         +0.0036          1202          1206          1202          1206
advanced/execute/main/sha1_shifts/5311_mean                                 +0.0038         +0.0038          2345          2354          2345          2354
advanced/execute/main/sha1_shifts/65536_mean                                +0.0040         +0.0040         28636         28751         28636         28752
advanced/execute/main/weierstrudel/0_mean                                   +0.0035         +0.0035           155           156           155           156
advanced/execute/main/weierstrudel/1_mean                                   +0.0043         +0.0043           337           339           337           339
advanced/execute/main/weierstrudel/3_mean                                   +0.0049         +0.0049           527           529           527           529
advanced/execute/main/weierstrudel/9_mean                                   +0.0043         +0.0043          1090          1094          1090          1094
advanced/execute/main/weierstrudel/14_mean                                  +0.0058         +0.0058          1559          1568          1559          1568
advanced/execute/micro/jump_around/empty_mean                               -0.1441         -0.1441           188           161           188           161
advanced/execute/micro/jumpdests_0xffff/empty_mean                          -0.0016         -0.0016           120           119           120           119
advanced/execute/micro/loop_with_many_jumpdests/empty_mean                  +0.0000         +0.0000         14611         14611         14611         14611
advanced/execute/micro/memory_grow_mload/nogrow_mean                        -0.0002         -0.0002            46            46            46            46
advanced/execute/micro/memory_grow_mload/by1_mean                           +0.0004         +0.0004            49            49            49            49
advanced/execute/micro/memory_grow_mload/by16_mean                          +0.0018         +0.0018            55            55            55            55
advanced/execute/micro/memory_grow_mload/by32_mean                          -0.0043         -0.0043            65            64            65            64
advanced/execute/micro/memory_grow_mstore/nogrow_mean                       +0.0004         +0.0004            50            50            50            50
advanced/execute/micro/memory_grow_mstore/by1_mean                          +0.0015         +0.0015            52            52            52            52
advanced/execute/micro/memory_grow_mstore/by16_mean                         +0.0079         +0.0079            58            59            58            59
advanced/execute/micro/memory_grow_mstore/by32_mean                         +0.0016         +0.0016            69            69            69            69
advanced/execute/micro/signextend/zero_mean                                 +0.0022         +0.0022            64            64            64            64
advanced/execute/micro/signextend/one_mean                                  +0.0020         +0.0020            64            64            64            64

@gumb0 gumb0 force-pushed the block-metadata-refactor branch 4 times, most recently from c08c338 to ef7b05e Compare April 12, 2022 15:17
@chfast chfast force-pushed the block-metadata-refactor branch from 290404c to 1c26b64 Compare April 12, 2022 18:42
@chfast chfast marked this pull request as ready for review April 12, 2022 18:45
@chfast chfast merged commit 0effa89 into master Apr 12, 2022
@chfast chfast deleted the block-metadata-refactor branch April 12, 2022 18:53
chfast added a commit that referenced this pull request Jun 14, 2022
Fix stack height check for the basic block following a JUMPI instruction.
Before the stack height was 1 too big because the implementation
incorrectly did not remove the JUMPI condition from the stack.

The bug was introduced in #457.
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.

Advanced: Improve basic block matadata placement
2 participants