Drop immediates array #618
Merged
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
chfast
force-pushed
the
immediates_merge
branch
from
80c9f1a to
c91d9d9
Compare
Codecov Report
@@ Coverage Diff @@
## master #618 +/- ##
==========================================
- Coverage 98.37% 98.36% -0.01%
==========================================
Files 69 69
Lines 9654 9622 -32
==========================================
- Hits 9497 9465 -32
Misses 157 157 |
axic
reviewed
Oct 22, 2020
chfast
force-pushed
the
immediates_merge
branch
3 times, most recently
from
b162348 to
656e46d
Compare
chfast
force-pushed
the
immediates_merge
branch
2 times, most recently
from
923bd69 to
ad8b7d3
Compare
axic
reviewed
Oct 22, 2020
| @@ -736,8 +730,9 @@ parser_result<Code> parse_expr(const uint8_t* pos, const uint8_t* end, FuncIdx f | |||
|
|
|||
| drop_operand(frame, operand_stack, find_local_type(func_inputs, locals, local_idx)); | |||
|
|
|||
| push(code.immediates, local_idx); | |||
| break; | |||
| code.instructions.push_back(opcode); | |||
There was a problem hiding this comment.
Why use push_back for these and not push(code.instructions, opcode)?
There was a problem hiding this comment.
This is the copy of line after the switch.
But I can reverse the question: Why use push(code.instructions, opcode) for these when code.instructions.push_back(opcode) is enough.
There was a problem hiding this comment.
Maybe push should be renamed to push_immediate, and optionally a new helper push_opcode added
There was a problem hiding this comment.
Maybe push should be renamed to push_immediate, and optionally a new helper push_opcode added
That would be nice for readability, in a separate PR.
axic
reviewed
Oct 22, 2020
| break; | ||
| code.instructions.push_back(opcode); | ||
| push(code.instructions, uint32_t{0}); // Diff to the else instruction | ||
| continue; |
There was a problem hiding this comment.
Why are many of these changed to continue?
There was a problem hiding this comment.
The continue skips the code.instructions.push_back(opcode); after the switch because you do it here before pushing the immediates.
There was a problem hiding this comment.
As opcode is always the first to be pushed, would it be better to move code.instructions.push_back(opcode); to before the switch?
There was a problem hiding this comment.
Possible currently, but I'm not sure this will stand long term. In #622 I present a way to skip noop instructions in parsing. Also we can replace return with br, etc. So I think the control what opcode is pushed if any on the instruction granularity is useful.
Alternatively, we can push the opcode, and the "pop" it or replace it in some cases.
axic
reviewed
Oct 22, 2020
axic
reviewed
Oct 22, 2020
chfast
commented
Oct 23, 2020
| break; | ||
| code.instructions.push_back(opcode); | ||
| push(code.instructions, uint32_t{0}); // Diff to the else instruction | ||
| continue; |
There was a problem hiding this comment.
The continue skips the code.instructions.push_back(opcode); after the switch because you do it here before pushing the immediates.
| @@ -736,8 +730,9 @@ parser_result<Code> parse_expr(const uint8_t* pos, const uint8_t* end, FuncIdx f | |||
|
|
|||
| drop_operand(frame, operand_stack, find_local_type(func_inputs, locals, local_idx)); | |||
|
|
|||
| push(code.immediates, local_idx); | |||
| break; | |||
| code.instructions.push_back(opcode); | |||
There was a problem hiding this comment.
This is the copy of line after the switch.
But I can reverse the question: Why use push(code.instructions, opcode) for these when code.instructions.push_back(opcode) is enough.
test/unittests/parser_expr_test.cpp
Outdated
| ElementsAre(Instr::i32_const, 0, 0, 0, 0, Instr::loop, Instr::br, /*arity:*/ 0, 0, 0, 0, | ||
| /*code_offset:*/ 5, 0, 0, 0, /*stack_drop:*/ 0, 0, 0, 0, Instr::end, Instr::drop, | ||
| Instr::end)); | ||
| // EXPECT_EQ(module_parent_stack->codesec[0].immediates, |
chfast
force-pushed
the
immediates_merge
branch
from
ad8b7d3 to
d3f0e65
Compare
gumb0
reviewed
Oct 23, 2020
lib/fizzy/parser_expr.cpp
Outdated
| const auto frame_type = frame.type; | ||
| auto frame_br_immediate_offsets = std::move(frame.br_immediate_offsets); | ||
|
|
||
| control_stack.pop(); | ||
| control_stack.emplace(Instr::else_, frame_type, static_cast<int>(operand_stack.size()), | ||
| code.instructions.size(), code.immediates.size()); | ||
| code.instructions.size()); | ||
| // br immediates from `then` branch will need to be filled at the end of `else` | ||
| control_stack.top().br_immediate_offsets = std::move(frame_br_immediate_offsets); | ||
|
|
||
| // Placeholders for immediate values, filled at the matching end instructions. |
There was a problem hiding this comment.
Move this comment one line down, to before pushing the immediate
Suggested change
| // Placeholders for immediate values, filled at the matching end instructions. | |
| // Placeholder for immediate value, filled at the matching end instructions. |
| @@ -736,8 +730,9 @@ parser_result<Code> parse_expr(const uint8_t* pos, const uint8_t* end, FuncIdx f | |||
|
|
|||
| drop_operand(frame, operand_stack, find_local_type(func_inputs, locals, local_idx)); | |||
|
|
|||
| push(code.immediates, local_idx); | |||
| break; | |||
| code.instructions.push_back(opcode); | |||
There was a problem hiding this comment.
Maybe push should be renamed to push_immediate, and optionally a new helper push_opcode added
lib/fizzy/parser_expr.cpp
Outdated
| push(code.immediates, uint32_t{0}); // Diff to the end instruction. | ||
| push(code.immediates, uint32_t{0}); // Diff for the immediates | ||
| code.instructions.push_back(opcode); | ||
| push(code.instructions, uint32_t{0}); // Diff to the end instruction. | ||
|
|
||
| // Fill in if's immediates with offsets of first instruction in else block. |
There was a problem hiding this comment.
Suggested change
| // Fill in if's immediates with offsets of first instruction in else block. | |
| // Fill in if's immediate with offset of first instruction in else block. |
lib/fizzy/parser_expr.cpp
Outdated
| @@ -541,26 +534,21 @@ parser_result<Code> parse_expr(const uint8_t* pos, const uint8_t* end, FuncIdx f | |||
| const auto target_pc = control_stack.size() == 1 ? | |||
| static_cast<uint32_t>(code.instructions.size()) : | |||
| static_cast<uint32_t>(code.instructions.size() + 1); | |||
| const auto target_imm = static_cast<uint32_t>(code.immediates.size()); | |||
|
|
|||
| if (frame.instruction == Instr::if_ || frame.instruction == Instr::else_) | |||
| { | |||
| // We're at the end instruction of the if block without else or at the end of | |||
| // else block. Fill in if/else's immediates with offsets of first instruction | |||
There was a problem hiding this comment.
Suggested change
| // else block. Fill in if/else's immediates with offsets of first instruction | |
| // else block. Fill in if/else's immediate with offset of first instruction |
|
My classic benchmarks, GCC10 LTO on Haswell 4GHz. The parsing got slower. There can be two reasons for that: we allocate differently - single array, and the parser loop is more complex. |
chfast
force-pushed
the
immediates_merge
branch
from
f365586 to
d3f0e65
Compare
chfast
commented
Oct 23, 2020
| @@ -363,10 +363,6 @@ struct Code | |||
| // The instructions bytecode without immediate values. | |||
| // https://webassembly.github.io/spec/core/binary/instructions.html | |||
| std::vector<uint8_t> instructions; | |||
There was a problem hiding this comment.
I also tried using bytes, but the performance is the same. So I'm happy to leave vector as it is simpler internally (string has SSO which is useless in this case).
gumb0
reviewed
Oct 23, 2020
| @@ -872,7 +872,7 @@ TEST(execute_control, if_else_smoke) | |||
|
|
|||
| const auto module = parse(bin); | |||
|
|
|||
| for (const auto param : {0u, 1u}) | |||
| for (const auto param : {0u}) | |||
gumb0
reviewed
Oct 23, 2020
| "00000000"_bytes; // stack_drop | ||
|
|
||
| EXPECT_EQ(code.immediates.substr(br_table_imm_offset, expected_br_imm.size()), expected_br_imm); | ||
| Instr::local_get, 0, 0, 0, 0, Instr::br_table, |
There was a problem hiding this comment.
These tests are super annoying, I'm not checking all the offsets
|
AMD EPYC 7601, GCC10, no LTO |
chfast
force-pushed
the
immediates_merge
branch
from
d3f0e65 to
963468b
Compare
chfast
force-pushed
the
immediates_merge
branch
from
0f5ed9d to
0723283
Compare
chfast
force-pushed
the
immediates_merge
branch
from
0723283 to
9f8929a
Compare
|
Haswell 4 GHz, GCC10 LTO |
|
AMD EPYC 7501 2 GHz, GCC10 no-LTO |
axic
reviewed
Nov 5, 2020
| @@ -320,33 +313,34 @@ TEST(parser_expr, instr_br_table) | |||
| (block | |||
| (block | |||
| (br_table 3 2 1 0 4 (get_local 0)) | |||
| (return (i32.const 99)) | |||
| (return (i32.const 0x41)) | |||
There was a problem hiding this comment.
Any particular reason these were renumbered? Just to make it easier to read below?
There was a problem hiding this comment.
I first converted them to hex to easier find them in the bytecode, but this seem not relevant any more.
axic
reviewed
Nov 5, 2020
|
|
||
| if (!module.has_memory()) | ||
| throw validation_error{"memory instructions require imported or defined memory"}; | ||
| break; | ||
| continue; |
There was a problem hiding this comment.
Is this continue here jumping to the while body? I did not know you can jump out from within switches with it.
There was a problem hiding this comment.
This is a bit hackish, but also "minimal" change. The continue only affects loops but break both loops and switches.
axic
reviewed
Nov 5, 2020
| @@ -861,11 +861,12 @@ parser_result<Code> parse_expr(const uint8_t* pos, const uint8_t* end, FuncIdx f | |||
|
|
|||
| uint32_t offset; | |||
| std::tie(offset, pos) = leb128u_decode<uint32_t>(pos, end); | |||
| push(code.immediates, offset); | |||
| code.instructions.push_back(opcode); | |||
There was a problem hiding this comment.
Btw, why is the old code (end of loop) using emplace_back(opcode) but the new commits are not?
There was a problem hiding this comment.
The std::string only has .push_back(). It will be easier to try this instead of std::vector.
axic
reviewed
Nov 5, 2020
| ASSERT_EQ(module->codesec[0].immediates.substr(4), "00000000"_bytes); // load offset. | ||
| auto* const load_instr = const_cast<uint8_t*>(&module->codesec[0].instructions[1]); | ||
| ASSERT_EQ(*load_instr, Instr::i32_load); | ||
| ASSERT_EQ(bytes_view(load_instr + 1, 4), "00000000"_bytes); // load offset. |
There was a problem hiding this comment.
Is this bytes_view alternative better because it is faster, or does it result in a nicer error display?
There was a problem hiding this comment.
I think it was easier for me to read it like that when modifying the test.
axic
reviewed
Nov 5, 2020
axic
reviewed
Nov 5, 2020
axic
reviewed
Nov 5, 2020
axic
reviewed
Nov 5, 2020
test/unittests/parser_expr_test.cpp
Outdated
| "00000000"_bytes; // stack_drop | ||
|
|
||
| EXPECT_EQ(code.immediates.substr(br_table_imm_offset, expected_br_imm.size()), expected_br_imm); | ||
| EXPECT_EQ(code.immediates.substr(0, expected_br_imm.size()), expected_br_imm); |
There was a problem hiding this comment.
Ah the old immediates table here skipped the local for some reason.
There was a problem hiding this comment.
This test always takes the part of the immediates for the br_table only. Now this part starts at 0 because we removed other immediates. I didn't bother to keep the br_table_imm_offset name. In the end when all immediates are move to instructions we check all bytes anyway.
axic
reviewed
Nov 5, 2020
| @@ -455,7 +455,7 @@ TEST(parser_expr, call_indirect_table_index) | |||
| const auto code1_bin = i32_const(0) + "1100000b"_bytes; | |||
| const auto [code, pos] = parse_expr(code1_bin, 0, {}, module); | |||
| EXPECT_THAT(code.instructions, | |||
| ElementsAre(Instr::i32_const, 0, 0, 0, 0, Instr::call_indirect, Instr::end)); | |||
| ElementsAre(Instr::i32_const, 0, 0, 0, 0, Instr::call_indirect, 0, 0, 0, 0, Instr::end)); | |||
There was a problem hiding this comment.
Is there really a single test affected by call immediates?
There was a problem hiding this comment.
It looks like. All intermediate commits should pass all tests. Immediates for calls are rather simple and at that time we were writing more wat2wasm tests.
axic
reviewed
Nov 5, 2020
test/unittests/parser_test.cpp
Outdated
| @@ -1461,5 +1456,4 @@ TEST(parser, milestone1) | |||
| ElementsAre(Instr::local_get, 0, 0, 0, 0, Instr::local_get, 1, 0, 0, 0, Instr::i32_add, | |||
| Instr::local_get, 2, 0, 0, 0, Instr::i32_add, Instr::local_tee, 2, 0, 0, 0, | |||
| Instr::local_get, 0, 0, 0, 0, Instr::i32_add, Instr::end)); | |||
| EXPECT_EQ(c.immediates.size(), 0); | |||
There was a problem hiding this comment.
Actually it would have been nice to have the final "remove immediates from struct" as a separate commit to see the impact of changes.
axic
force-pushed
the
immediates_merge
branch
from
9f8929a to
05023ef
Compare
Co-authored-by: Paweł Bylica <[email protected]>
This also drop the immediates array as no longer needed.
axic
force-pushed
the
immediates_merge
branch
from
05023ef to
1bb954b
Compare
axic
approved these changes
Nov 6, 2020
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.
No description provided.