Single arity immediate per br_table instruction and make arity immediate 4-byte wide #354
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Codecov Report
@@ Coverage Diff @@
## master #354 +/- ##
=======================================
Coverage 99.55% 99.55%
=======================================
Files 49 49
Lines 14529 14535 +6
=======================================
+ Hits 14464 14470 +6
Misses 65 65 |
|
This should be benchmarked. |
chfast
approved these changes
Jun 1, 2020
|
There is 5% regression on mul benchmark: |
chfast
requested changes
Jun 1, 2020
gumb0
force-pushed
the
br-table-imm-arity
branch
2 times, most recently
from
58baadd
to
cdcf9af
Compare
|
Basically this change gives no speed benefit, just penalty on the mul256 benchmark. Should we merge this? |
|
|
I don't think llvm uses |
chfast
force-pushed
the
br-table-imm-arity
branch
from
cdcf9af
to
cca8042
Compare
|
I don't have good news. These are execution benchmarks results. I removed statistically insignificant results, except for eli_interpreter (as it is relevant for br_table). I also repeated keccak and eli_interpreter cases separately, but the results were the same or worse. The issue is with keccak, and it actually uses |
|
On laptop/skylake seems to be no regression. no_turbo: turbo (unstable results): Maybe change in immediates layout affects timings on memory loads (e.g. some immediates accesses cross cache line). I can also see increased branch prediction misses from 2% to 3% on Haswell (first hardware setup). I'm recommending creating a separate commit where only arity immediate value order is changed. |
The first commit here does only reordering. |
|
The reordering commit does not change anything. The "Tweak performance 1" lowers the regression by ~2x. This PR should really not touch any instruction except |
|
So I also dumped alignments of 4-byte reads in |
|
The "Tweak performance 3" make all reads aligned, but there is still 3-4% performance regression. |
|
I benchmarked every commit once again on Keccak workload and compared that with immediates alignment, but this rather does not explain the timings: The |
|
In #376 I checked what is the impact of only changing arity immediate value from |
chfast
force-pushed
the
br-table-imm-arity
branch
from
ee0459c
to
d31ab80
Compare
|
I removed last commit because was to invasive ( This can land in current form. |
chfast
force-pushed
the
br-table-imm-arity
branch
from
d31ab80
to
e567b36
Compare
|
I checked the assembly before and after. Looks like for GCC the issues is with |
chfast
requested changes
Jun 25, 2020
gumb0
force-pushed
the
br-table-imm-arity
branch
from
3195ab0
to
279e58a
Compare
|
Rebased. |
|
@chfast can you run the benchmarks again? Perhaps numbers are different now after a month's worth of changes. |
axic
reviewed
Jul 28, 2020
lib/fizzy/execute.cpp
Outdated
| @@ -670,27 +669,31 @@ ExecutionResult execute(Instance& instance, FuncIdx func_idx, span<const uint64_ | |||
| case Instr::br_if: | |||
| case Instr::return_: | |||
| { | |||
| const auto arity = read<uint8_t>(immediates); | |||
There was a problem hiding this comment.
Could also check if making this uint32_t makes any difference, as attempted in #376.
There was a problem hiding this comment.
Pushed variant with uint32_t arity.
axic
reviewed
Jul 28, 2020
lib/fizzy/execute.cpp
Outdated
| break; | ||
| } | ||
| case Instr::br_table: | ||
| { | ||
| const auto br_table_size = read<uint32_t>(immediates); | ||
| const auto arity = read<uint8_t>(immediates); |
|
|
With arity immediate being 4 bytes, this is nice. |
chfast
approved these changes
Jul 29, 2020
gumb0
commented
Jul 29, 2020
test/unittests/parser_expr_test.cpp
Outdated
| } | ||
|
|
||
| TEST(parser_expr, instr_br_table) | ||
| TEST(parser_expr, DISABLED_instr_br_table) |
There was a problem hiding this comment.
This test is annoying to update, should I just delete it?
gumb0
force-pushed
the
br-table-imm-arity
branch
from
03107a2
to
e0ae51d
Compare
|
Changed |
gumb0
force-pushed
the
br-table-imm-arity
branch
from
e0ae51d
to
0c795c7
Compare
axic
approved these changes
Jul 29, 2020
gumb0
changed the title
Merged
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Get rid of duplicated arity in
br_tableimmediates.