From 5987486862b8c989452bc62d359168a5686b462e Mon Sep 17 00:00:00 2001 From: "Kartik K. Agaram" Date: Sun, 28 May 2017 14:28:07 -0700 Subject: 3887 - clean up early exits in interpreter loop It's always confusing when `break` refers to a `switch` but `continue` refers to the loop around the `switch`. But we've done ugly things like this and `goto` for expedience. However, we're starting to run into cases where we now need to insert code at every `continue` or `continue`-mimicking `goto` inside the core interpreter loop. Better to make the loop single-entry-single-exit. Common things to run after every instruction will now happen inside the `finish_instruction` function rather than at the `finish_instruction` label. --- 020run.cc | 28 ++++++++++++++++++---------- 024jump.cc | 15 ++++++++++++--- 026call.cc | 27 +++++++++++++-------------- 030container.cc | 3 ++- 032array.cc | 6 ++++-- 033exclusive_container.cc | 3 ++- 071recipe.cc | 8 ++++++-- 7 files changed, 57 insertions(+), 33 deletions(-) diff --git a/020run.cc b/020run.cc index 084cafbb..15644843 100644 --- a/020run.cc +++ b/020run.cc @@ -80,6 +80,12 @@ void run_current_routine() { } // instructions below will write to 'products' vector > products; + //: This will be a large switch that later layers will often insert cases + //: into. Never call 'continue' within it. Instead, we'll explicitly + //: control which of the following stages after the switch we run for each + //: instruction. + bool write_products = true; + bool fall_through_to_next_instruction = true; switch (current_instruction().operation) { // Primitive Recipe Implementations case COPY: { @@ -92,18 +98,20 @@ void run_current_routine() { } } //: used by a later layer - Writing_products_of_instruction = true; - if (SIZE(products) < SIZE(current_instruction().products)) { - raise << SIZE(products) << " vs " << SIZE(current_instruction().products) << ": failed to write to all products in '" << to_original_string(current_instruction()) << "'\n" << end(); - } - else { - for (int i = 0; i < SIZE(current_instruction().products); ++i) - write_memory(current_instruction().products.at(i), products.at(i)); + if (write_products) { + Writing_products_of_instruction = true; + if (SIZE(products) < SIZE(current_instruction().products)) { + raise << SIZE(products) << " vs " << SIZE(current_instruction().products) << ": failed to write to all products in '" << to_original_string(current_instruction()) << "'\n" << end(); + } + else { + for (int i = 0; i < SIZE(current_instruction().products); ++i) + write_memory(current_instruction().products.at(i), products.at(i)); + } + Writing_products_of_instruction = false; } - Writing_products_of_instruction = false; // End Running One Instruction - finish_instruction:; - ++current_step_index(); + if (fall_through_to_next_instruction) + ++current_step_index(); } stop_running_current_routine:; } diff --git a/024jump.cc b/024jump.cc index 5e345eca..748b2405 100644 --- a/024jump.cc +++ b/024jump.cc @@ -30,7 +30,10 @@ case JUMP: { assert(current_instruction().ingredients.at(0).initialized); current_step_index() += ingredients.at(0).at(0)+1; trace(9998, "run") << "jumping to instruction " << current_step_index() << end(); - continue; // skip rest of this instruction + // skip rest of this instruction + write_products = false; + fall_through_to_next_instruction = false; + break; } //: special type to designate jump targets @@ -78,7 +81,10 @@ case JUMP_IF: { } current_step_index() += ingredients.at(1).at(0)+1; trace(9998, "run") << "jumping to instruction " << current_step_index() << end(); - continue; // skip rest of this instruction + // skip rest of this instruction + write_products = false; + fall_through_to_next_instruction = false; + break; } :(scenario jump_if) @@ -131,7 +137,10 @@ case JUMP_UNLESS: { } current_step_index() += ingredients.at(1).at(0)+1; trace(9998, "run") << "jumping to instruction " << current_step_index() << end(); - continue; // skip rest of this instruction + // skip rest of this instruction + write_products = false; + fall_through_to_next_instruction = false; + break; } :(scenario jump_unless) diff --git a/026call.cc b/026call.cc index dd9db69c..81012350 100644 --- a/026call.cc +++ b/026call.cc @@ -107,21 +107,20 @@ if (!contains_key(Recipe, inst.operation)) { } :(replace{} "default:" following "End Primitive Recipe Implementations") default: { - const instruction& call_instruction = current_instruction(); - if (Recipe.find(current_instruction().operation) == Recipe.end()) { // duplicate from Checks - // stop running this instruction immediately - ++current_step_index(); - continue; + if (contains_key(Recipe, current_instruction().operation)) { // error already raised in Checks above + // not a primitive; look up the book of recipes + if (Trace_stream) { + ++Trace_stream->callstack_depth; + trace(9999, "trace") << "incrementing callstack depth to " << Trace_stream->callstack_depth << end(); + assert(Trace_stream->callstack_depth < 9000); // 9998-101 plus cushion + } + const instruction& call_instruction = current_instruction(); + Current_routine->calls.push_front(call(current_instruction().operation)); + finish_call_housekeeping(call_instruction, ingredients); + // not done with caller + write_products = false; + fall_through_to_next_instruction = false; } - // not a primitive; look up the book of recipes - if (Trace_stream) { - ++Trace_stream->callstack_depth; - trace(9999, "trace") << "incrementing callstack depth to " << Trace_stream->callstack_depth << end(); - assert(Trace_stream->callstack_depth < 9000); // 9998-101 plus cushion - } - Current_routine->calls.push_front(call(current_instruction().operation)); - finish_call_housekeeping(call_instruction, ingredients); - continue; // not done with caller; don't increment step_index of caller } :(code) void finish_call_housekeeping(const instruction& call_instruction, const vector >& ingredients) { diff --git a/030container.cc b/030container.cc index 4cb147fe..b189435f 100644 --- a/030container.cc +++ b/030container.cc @@ -575,11 +575,12 @@ case PUT: { // optimization: directly write the element rather than updating 'product' // and writing the entire container // Write Memory in PUT in Run + write_products = false; for (int i = 0; i < SIZE(ingredients.at(2)); ++i) { trace(9999, "mem") << "storing " << no_scientific(ingredients.at(2).at(i)) << " in location " << address+i << end(); put(Memory, address+i, ingredients.at(2).at(i)); } - goto finish_instruction; + break; } :(scenario put_product_error) diff --git a/032array.cc b/032array.cc index 778daa97..3b521263 100644 --- a/032array.cc +++ b/032array.cc @@ -66,7 +66,8 @@ case CREATE_ARRAY: { put(Memory, base_address+i, 0); } // no need to update product - goto finish_instruction; + write_products = false; + break; } :(scenario copy_array) @@ -557,13 +558,14 @@ case PUT_INDEX: { trace(9998, "run") << "address to copy to is " << address << end(); // optimization: directly write the element rather than updating 'product' // and writing the entire array + write_products = false; vector value = read_memory(current_instruction().ingredients.at(2)); // Write Memory in PUT_INDEX in Run for (int i = 0; i < SIZE(value); ++i) { trace(9999, "mem") << "storing " << no_scientific(value.at(i)) << " in location " << address+i << end(); put(Memory, address+i, value.at(i)); } - goto finish_instruction; + break; } :(scenario put_index_out_of_bounds) diff --git a/033exclusive_container.cc b/033exclusive_container.cc index eee16643..86dc1c8a 100644 --- a/033exclusive_container.cc +++ b/033exclusive_container.cc @@ -160,6 +160,7 @@ case MAYBE_CONVERT: { reagent/*copy*/ status = current_instruction().products.at(1); // Update MAYBE_CONVERT status in Run // optimization: directly write results to only update first product when necessary + write_products = false; if (tag == static_cast(get_or_insert(Memory, base_address))) { const reagent& variant = variant_type(base, tag); trace(9999, "mem") << "storing 1 in location " << status.value << end(); @@ -177,7 +178,7 @@ case MAYBE_CONVERT: { trace(9999, "mem") << "storing 0 in location " << status.value << end(); put(Memory, status.value, 0); } - goto finish_instruction; + break; } :(code) diff --git a/071recipe.cc b/071recipe.cc index 1a145ed6..c44d3127 100644 --- a/071recipe.cc +++ b/071recipe.cc @@ -114,13 +114,17 @@ case CALL: { raise << maybe(current_recipe_name()) << "tried to call empty recipe in '" << to_string(current_instruction()) << "'" << end(); break; } - instruction/*copy*/ call_instruction = current_instruction(); + const call& caller_frame = current_call(); + instruction/*copy*/ call_instruction = to_instruction(caller_frame); call_instruction.operation = ingredients.at(0).at(0); call_instruction.ingredients.erase(call_instruction.ingredients.begin()); Current_routine->calls.push_front(call(ingredients.at(0).at(0))); ingredients.erase(ingredients.begin()); // drop the callee finish_call_housekeeping(call_instruction, ingredients); - continue; + // not done with caller + write_products = false; + fall_through_to_next_instruction = false; + break; } //:: check types for 'call' instructions -- cgit 1.4.1-2-gfad0