Move branch immediate values to the instructions array

chfast · chfast · commit 1b84ab76fac8 · 2020-11-05T20:18:06.000+01: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);
@@ -523,7 +520,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)
     {
@@ -539,27 +535,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 immediate 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:
@@ -573,16 +562,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:
@@ -595,9 +584,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,28 @@ 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);
+
+            // Placeholder for the immediate value, filled at the matching end instructions.
+            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());
+            // Fill in if's immediate with the offset of first instruction in else block.
+            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 +535,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
+                    // else block. Fill in if/else's immediate with the offset 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 +577,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 +597,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 +635,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 +658,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 interleaved with decoded 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_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
