Move branch immediate values to the instructions array

chfast · chfast · commit 656e46dc4a2a · 2020-10-22T20:13:56.000+02:00
This also drop the immediates array as no longer needed.
diff --git a/lib/fizzy/execute.cpp b/lib/fizzy/execute.cpp
@@ -18,8 +18,8 @@ namespace fizzy
 {
 namespace
 {
-// code_offset + imm_offset + stack_height
-constexpr auto BranchImmediateSize = 3 * sizeof(uint32_t);
+// code_offset + stack_drop
+constexpr auto BranchImmediateSize = 2 * sizeof(uint32_t);
 
 constexpr uint32_t F32AbsMask = 0x7fffffff;
 constexpr uint32_t F32SignMask = ~F32AbsMask;
@@ -453,15 +453,12 @@ __attribute__((no_sanitize("float-cast-overflow"))) inline constexpr float demot
     return static_cast<float>(value);
 }
 
-void branch(const Code& code, OperandStack& stack, const uint8_t*& pc, const uint8_t*& immediates,
-    uint32_t arity) noexcept
+void branch(const Code& code, OperandStack& stack, const uint8_t*& pc, uint32_t arity) noexcept
 {
-    const auto code_offset = read<uint32_t>(immediates);
-    const auto imm_offset = read<uint32_t>(immediates);
-    const auto stack_drop = read<uint32_t>(immediates);
+    const auto code_offset = read<uint32_t>(pc);
+    const auto stack_drop = read<uint32_t>(pc);
 
     pc = code.instructions.data() + code_offset;
-    immediates = code.immediates.data() + imm_offset;
 
     // When branch is taken, additional stack items must be dropped.
     assert(static_cast<int>(stack_drop) >= 0);
@@ -532,7 +529,6 @@ ExecutionResult execute(Instance& instance, FuncIdx func_idx, const Value* args,
         static_cast<size_t>(code.max_stack_height));
 
     const uint8_t* pc = code.instructions.data();
-    const uint8_t* immediates = code.immediates.data();
 
     while (true)
     {
@@ -548,27 +544,20 @@ ExecutionResult execute(Instance& instance, FuncIdx func_idx, const Value* args,
         case Instr::if_:
         {
             if (stack.pop().as<uint32_t>() != 0)
-                immediates += 2 * sizeof(uint32_t);  // Skip the immediates for else instruction.
+                pc += sizeof(uint32_t);  // Skip the immediates for else instruction.
             else
             {
-                const auto target_pc = read<uint32_t>(immediates);
-                const auto target_imm = read<uint32_t>(immediates);
-
+                const auto target_pc = read<uint32_t>(pc);
                 pc = code.instructions.data() + target_pc;
-                immediates = code.immediates.data() + target_imm;
             }
             break;
         }
         case Instr::else_:
         {
             // We reach else only after executing if block ("then" part),
             // so we need to skip else block now.
-            const auto target_pc = read<uint32_t>(immediates);
-            const auto target_imm = read<uint32_t>(immediates);
-
+            const auto target_pc = read<uint32_t>(pc);
             pc = code.instructions.data() + target_pc;
-            immediates = code.immediates.data() + target_imm;
-
             break;
         }
         case Instr::end:
@@ -582,16 +571,16 @@ ExecutionResult execute(Instance& instance, FuncIdx func_idx, const Value* args,
         case Instr::br_if:
         case Instr::return_:
         {
-            const auto arity = read<uint32_t>(immediates);
+            const auto arity = read<uint32_t>(pc);
 
             // Check condition for br_if.
             if (instruction == Instr::br_if && stack.pop().as<uint32_t>() == 0)
             {
-                immediates += BranchImmediateSize;
+                pc += BranchImmediateSize;
                 break;
             }
 
-            branch(code, stack, pc, immediates, arity);
+            branch(code, stack, pc, arity);
             break;
         }
         case Instr::br_table:
@@ -604,9 +593,9 @@ ExecutionResult execute(Instance& instance, FuncIdx func_idx, const Value* args,
             const auto label_idx_offset = br_table_idx < br_table_size ?
                                               br_table_idx * BranchImmediateSize :
                                               br_table_size * BranchImmediateSize;
-            immediates += label_idx_offset;
+            pc += label_idx_offset;
 
-            branch(code, stack, pc, immediates, arity);
+            branch(code, stack, pc, arity);
             break;
         }
         case Instr::call:
diff --git a/lib/fizzy/parser_expr.cpp b/lib/fizzy/parser_expr.cpp
@@ -19,14 +19,6 @@ inline void store(uint8_t* dst, T value) noexcept
     __builtin_memcpy(dst, &value, sizeof(value));
 }
 
-template <typename T>
-inline void push(bytes& b, T value)
-{
-    uint8_t storage[sizeof(T)];
-    store(storage, value);
-    b.append(storage, sizeof(storage));
-}
-
 template <typename T>
 inline void push(std::vector<uint8_t>& b, T value)
 {
@@ -48,9 +40,6 @@ struct ControlFrame
     /// The target instruction code offset.
     const size_t code_offset{0};
 
-    /// The immediates offset for block instructions.
-    const size_t immediates_offset{0};
-
     /// The frame stack height of the parent frame.
     const int parent_stack_height{0};
 
@@ -62,11 +51,10 @@ struct ControlFrame
     std::vector<size_t> br_immediate_offsets{};
 
     ControlFrame(Instr _instruction, std::optional<ValType> _type, int _parent_stack_height,
-        size_t _code_offset = 0, size_t _immediates_offset = 0) noexcept
+        size_t _code_offset = 0) noexcept
       : instruction{_instruction},
         type{_type},
         code_offset{_code_offset},
-        immediates_offset{_immediates_offset},
         parent_stack_height{_parent_stack_height}
     {}
 };
@@ -210,16 +198,16 @@ inline void update_branch_stack(const ControlFrame& current_frame, const Control
         drop_operand(current_frame, operand_stack, from_valtype(*branch_frame_type));
 }
 
-void push_branch_immediates(const ControlFrame& branch_frame, int stack_height, bytes& immediates)
+void push_branch_immediates(
+    const ControlFrame& branch_frame, int stack_height, std::vector<uint8_t>& instructions)
 {
     // How many stack items to drop when taking the branch.
     const auto stack_drop = stack_height - branch_frame.parent_stack_height;
 
     // Push frame start location as br immediates - these are final if frame is loop,
     // but for block/if/else these are just placeholders, to be filled at end instruction.
-    push(immediates, static_cast<uint32_t>(branch_frame.code_offset));
-    push(immediates, static_cast<uint32_t>(branch_frame.immediates_offset));
-    push(immediates, static_cast<uint32_t>(stack_drop));
+    push(instructions, static_cast<uint32_t>(branch_frame.code_offset));
+    push(instructions, static_cast<uint32_t>(stack_drop));
 }
 
 inline void mark_frame_unreachable(
@@ -473,7 +461,7 @@ parser_result<Code> parse_expr(const uint8_t* pos, const uint8_t* end, FuncIdx f
 
             // Push label with immediates offset after arity.
             control_stack.emplace(Instr::block, block_type, static_cast<int>(operand_stack.size()),
-                code.instructions.size(), code.immediates.size());
+                code.instructions.size());
             break;
         }
 
@@ -483,7 +471,7 @@ parser_result<Code> parse_expr(const uint8_t* pos, const uint8_t* end, FuncIdx f
             std::tie(loop_type, pos) = parse_blocktype(pos, end);
 
             control_stack.emplace(Instr::loop, loop_type, static_cast<int>(operand_stack.size()),
-                code.instructions.size(), code.immediates.size());
+                code.instructions.size());
             break;
         }
 
@@ -493,12 +481,12 @@ parser_result<Code> parse_expr(const uint8_t* pos, const uint8_t* end, FuncIdx f
             std::tie(if_type, pos) = parse_blocktype(pos, end);
 
             control_stack.emplace(Instr::if_, if_type, static_cast<int>(operand_stack.size()),
-                code.instructions.size(), code.immediates.size());
+                code.instructions.size());
 
             // Placeholders for immediate values, filled at the matching end or else instructions.
-            push(code.immediates, uint32_t{0});  // Diff to the else instruction
-            push(code.immediates, uint32_t{0});  // Diff for the immediates.
-            break;
+            code.instructions.push_back(opcode);
+            push(code.instructions, uint32_t{0});  // Diff to the else instruction
+            continue;
         }
 
         case Instr::else_:
@@ -508,30 +496,27 @@ parser_result<Code> parse_expr(const uint8_t* pos, const uint8_t* end, FuncIdx f
 
             update_result_stack(frame, operand_stack);  // else is the end of if.
 
-            const auto if_imm_offset = frame.immediates_offset;
+            const auto if_imm_offset = frame.code_offset + 1;
             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.
-            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.
-            const auto target_pc = static_cast<uint32_t>(code.instructions.size() + 1);
-            const auto target_imm = static_cast<uint32_t>(code.immediates.size());
+            const auto target_pc = static_cast<uint32_t>(code.instructions.size());
 
-            // Set the imm values for else instruction.
-            auto* if_imm = code.immediates.data() + if_imm_offset;
+            // Set the imm values for if instruction.
+            auto* if_imm = code.instructions.data() + if_imm_offset;
             store(if_imm, target_pc);
-            if_imm += sizeof(target_pc);
-            store(if_imm, target_imm);
-            break;
+            continue;
         }
 
         case Instr::end:
@@ -549,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
                     // after if/else block.
-                    auto* if_imm = code.immediates.data() + frame.immediates_offset;
+                    auto* if_imm = code.instructions.data() + frame.code_offset + 1;
                     store(if_imm, target_pc);
-                    if_imm += sizeof(target_pc);
-                    store(if_imm, target_imm);
                 }
 
                 // Fill in immediates all br/br_table instructions jumping out of this block.
                 for (const auto br_imm_offset : frame.br_immediate_offsets)
                 {
-                    auto* br_imm = code.immediates.data() + br_imm_offset;
+                    auto* br_imm = code.instructions.data() + br_imm_offset;
                     store(br_imm, static_cast<uint32_t>(target_pc));
-                    br_imm += sizeof(uint32_t);
-                    store(br_imm, static_cast<uint32_t>(target_imm));
                     // stack drop and arity were already stored in br handler
                 }
             }
@@ -596,13 +576,14 @@ parser_result<Code> parse_expr(const uint8_t* pos, const uint8_t* end, FuncIdx f
 
             update_branch_stack(frame, branch_frame, operand_stack);
 
-            push(code.immediates, get_branch_arity(branch_frame));
+            code.instructions.push_back(opcode);
+            push(code.instructions, get_branch_arity(branch_frame));
 
             // Remember this br immediates offset to fill it at end instruction.
-            branch_frame.br_immediate_offsets.push_back(code.immediates.size());
+            branch_frame.br_immediate_offsets.push_back(code.instructions.size());
 
             push_branch_immediates(
-                branch_frame, static_cast<int>(operand_stack.size()), code.immediates);
+                branch_frame, static_cast<int>(operand_stack.size()), code.instructions);
 
             if (instr == Instr::br)
                 mark_frame_unreachable(frame, operand_stack);
@@ -615,7 +596,7 @@ parser_result<Code> parse_expr(const uint8_t* pos, const uint8_t* end, FuncIdx f
                     push_operand(operand_stack, *branch_frame.type);
             }
 
-            break;
+            continue;
         }
 
         case Instr::br_table:
@@ -653,13 +634,13 @@ parser_result<Code> parse_expr(const uint8_t* pos, const uint8_t* end, FuncIdx f
                 if (get_branch_frame_type(branch_frame) != default_branch_type)
                     throw validation_error{"br_table labels have inconsistent types"};
 
-                branch_frame.br_immediate_offsets.push_back(code.immediates.size());
+                branch_frame.br_immediate_offsets.push_back(code.instructions.size());
                 push_branch_immediates(
-                    branch_frame, static_cast<int>(operand_stack.size()), code.immediates);
+                    branch_frame, static_cast<int>(operand_stack.size()), code.instructions);
             }
-            default_branch_frame.br_immediate_offsets.push_back(code.immediates.size());
+            default_branch_frame.br_immediate_offsets.push_back(code.instructions.size());
             push_branch_immediates(
-                default_branch_frame, static_cast<int>(operand_stack.size()), code.immediates);
+                default_branch_frame, static_cast<int>(operand_stack.size()), code.instructions);
 
             mark_frame_unreachable(frame, operand_stack);
 
@@ -676,15 +657,16 @@ parser_result<Code> parse_expr(const uint8_t* pos, const uint8_t* end, FuncIdx f
 
             update_branch_stack(frame, branch_frame, operand_stack);
 
-            push(code.immediates, get_branch_arity(branch_frame));
+            code.instructions.push_back(opcode);
+            push(code.instructions, get_branch_arity(branch_frame));
 
-            branch_frame.br_immediate_offsets.push_back(code.immediates.size());
+            branch_frame.br_immediate_offsets.push_back(code.instructions.size());
 
             push_branch_immediates(
-                branch_frame, static_cast<int>(operand_stack.size()), code.immediates);
+                branch_frame, static_cast<int>(operand_stack.size()), code.instructions);
 
             mark_frame_unreachable(frame, operand_stack);
-            break;
+            continue;
         }
 
         case Instr::call:
diff --git a/lib/fizzy/types.hpp b/lib/fizzy/types.hpp
@@ -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;
-
-    // The decoded instructions' immediate values.
-    // These are instruction-type dependent fixed size value in the order of instructions.
-    bytes immediates;
 };
 
 /// The reference to the `code` in the wasm binary.
diff --git a/test/unittests/execute_control_test.cpp b/test/unittests/execute_control_test.cpp
@@ -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})
     {
         constexpr uint64_t expected_results[]{
             2,  // else branch.
diff --git a/test/unittests/execute_numeric_test.cpp b/test/unittests/execute_numeric_test.cpp
@@ -22,8 +22,7 @@ ExecutionResult execute_unary_operation(Instr instr, uint64_t arg)
     module->funcsec.emplace_back(TypeIdx{0});
     module->codesec.emplace_back(Code{1, 0,
         {static_cast<uint8_t>(Instr::local_get), 0, 0, 0, 0, static_cast<uint8_t>(instr),
-            static_cast<uint8_t>(Instr::end)},
-        {}});
+            static_cast<uint8_t>(Instr::end)}});
 
     return execute(*instantiate(std::move(module)), 0, {arg});
 }
@@ -36,8 +35,7 @@ ExecutionResult execute_binary_operation(Instr instr, uint64_t lhs, uint64_t rhs
     module->funcsec.emplace_back(TypeIdx{0});
     module->codesec.emplace_back(Code{2, 0,
         {static_cast<uint8_t>(Instr::local_get), 0, 0, 0, 0, static_cast<uint8_t>(Instr::local_get),
-            1, 0, 0, 0, static_cast<uint8_t>(instr), static_cast<uint8_t>(Instr::end)},
-        {}});
+            1, 0, 0, 0, static_cast<uint8_t>(instr), static_cast<uint8_t>(Instr::end)}});
 
     return execute(*instantiate(std::move(module)), 0, {lhs, rhs});
 }
diff --git a/test/unittests/parser_expr_test.cpp b/test/unittests/parser_expr_test.cpp
diff --git a/test/unittests/parser_test.cpp b/test/unittests/parser_test.cpp

Original file line number	Diff line number	Diff line change
`@@ -872,7 +872,7 @@ TEST(execute_control, if_else_smoke)`
`872`	`872`
`873`	`873`	`const auto module = parse(bin);`
`874`	`874`
`875`		`- for (const auto param : {0u, 1u})`
	`875`	`+ for (const auto param : {0u})`
`876`	`876`	`{`
`877`	`877`	`constexpr uint64_t expected_results[]{`
`878`	`878`	`2, // else branch.`