Creating temps for each arg. (aptos-labs#15514)

Author: vineethk

third_party/move/move-compiler-v2/src/bytecode_generator.rs

@@ -1110,7 +1110,14 @@ impl<'env> Generator<'env> {
     fn gen_arg_list(&mut self, exps: &[Exp]) -> Vec<TempIndex> {
         // If all args are side-effect free, we don't need to force temporary generation
         // to get left-to-right evaluation.
-        let with_forced_temp = !exps.iter().all(is_definitely_pure);
+        // TODO: after comparison testing, remove depending on the experiment and always
+        // have `with_forced_temp` be true.
+        let options = self
+            .env()
+            .get_extension::<Options>()
+            .expect("Options is available");
+        let with_forced_temp = options.experiment_on(Experiment::RETAIN_TEMPS_FOR_ARGS)
+            || !exps.iter().all(is_definitely_pure);
         let len = exps.len();
         // Generate code with (potentially) forced creation of temporaries for all except last arg.
         let mut args = exps

third_party/move/move-compiler-v2/src/experiments.rs

@@ -276,6 +276,14 @@ pub static EXPERIMENTS: Lazy<BTreeMap<String, Experiment>> = Lazy::new(|| {
             description: "Avoid storing to a local during assigns".to_string(),
             default: Inherited(Experiment::OPTIMIZE_WAITING_FOR_COMPARE_TESTS.to_string()),
         },
+        Experiment {
+            name: Experiment::RETAIN_TEMPS_FOR_ARGS.to_string(),
+            description:
+                "Create temps for each argument of a function call during stackless bytecode \
+                 generation and retain them until file format bytecode generation"
+                    .to_string(),
+            default: Inherited(Experiment::OPTIMIZE_WAITING_FOR_COMPARE_TESTS.to_string()),
+        },
     ];
     experiments
         .into_iter()
@@ -316,6 +324,7 @@ impl Experiment {
     pub const RECURSIVE_TYPE_CHECK: &'static str = "recursive-type-check";
     pub const REFERENCE_SAFETY: &'static str = "reference-safety";
     pub const REFERENCE_SAFETY_V3: &'static str = "reference-safety-v3";
+    pub const RETAIN_TEMPS_FOR_ARGS: &'static str = "retain-temps-for-args";
     pub const SEQS_IN_BINOPS_CHECK: &'static str = "seqs-in-binops-check";
     pub const SPEC_CHECK: &'static str = "spec-check";
     pub const SPEC_REWRITE: &'static str = "spec-rewrite";

third_party/move/move-compiler-v2/src/lib.rs

@@ -465,7 +465,7 @@ pub fn bytecode_pipeline(env: &GlobalEnv) -> FunctionTargetPipeline {
         pipeline.add_processor(Box::new(UnreachableCodeProcessor {}));
         pipeline.add_processor(Box::new(UnreachableCodeRemover {}));
         pipeline.add_processor(Box::new(LiveVarAnalysisProcessor::new(true)));
-        pipeline.add_processor(Box::new(DeadStoreElimination {}));
+        pipeline.add_processor(Box::new(DeadStoreElimination::new(true)));
     }
 
     if options.experiment_on(Experiment::VARIABLE_COALESCING) {
@@ -484,7 +484,11 @@ pub fn bytecode_pipeline(env: &GlobalEnv) -> FunctionTargetPipeline {
 
     if options.experiment_on(Experiment::DEAD_CODE_ELIMINATION) {
         pipeline.add_processor(Box::new(LiveVarAnalysisProcessor::new(true)));
-        pipeline.add_processor(Box::new(DeadStoreElimination {}));
+        // TODO: after comparison testing passes, always call with `eliminate_all_self_assigns` set
+        // to `false` when instantiating `DeadStoreElimination`.
+        pipeline.add_processor(Box::new(DeadStoreElimination::new(
+            !options.experiment_on(Experiment::RETAIN_TEMPS_FOR_ARGS),
+        )));
     }
 
     // Run live var analysis again because it could be invalidated by previous pipeline steps,

third_party/move/move-compiler-v2/src/pipeline/dead_store_elimination.rs

@@ -14,6 +14,8 @@
 //! this transformation removes dead stores, i.e., assignments and loads to locals which
 //! are not live afterwards (or are live only in dead code, making them effectively dead).
 //! In addition, it also removes self-assignments, i.e., assignments of the form `x = x`.
+//! One can also remove only those self-assignments where the definition is in the same block
+//! before the self-assign by using `eliminate_all_self_assigns=false`.
 
 use crate::pipeline::livevar_analysis_processor::LiveVarAnnotation;
 use move_binary_format::file_format::CodeOffset;
@@ -22,6 +24,7 @@ use move_stackless_bytecode::{
     function_target::{FunctionData, FunctionTarget},
     function_target_pipeline::{FunctionTargetProcessor, FunctionTargetsHolder},
     stackless_bytecode::Bytecode,
+    stackless_control_flow_graph::StacklessControlFlowGraph,
 };
 use std::collections::{BTreeMap, BTreeSet};
 
@@ -59,19 +62,26 @@ struct ReducedDefUseGraph {
 
 impl ReducedDefUseGraph {
     /// Get the dead stores that are safe to remove from the function `target`.
-    pub fn dead_stores(target: &FunctionTarget) -> BTreeSet<u16> {
+    /// If `eliminate_all_self_assigns` is true, all self-assignments are removed.
+    pub fn dead_stores(target: &FunctionTarget, eliminate_all_self_assigns: bool) -> BTreeSet<u16> {
         Self {
             children: BTreeMap::new(),
             parents: BTreeMap::new(),
             defs_alive: BTreeSet::new(),
             defs_dead: BTreeSet::new(),
         }
-        .run_stages(target)
+        .run_stages(target, eliminate_all_self_assigns)
     }
 
     /// Run various stages to return the dead stores from `target`.
-    fn run_stages(mut self, target: &FunctionTarget) -> BTreeSet<u16> {
+    /// If `eliminate_all_self_assigns` is true, all self-assignments are removed.
+    fn run_stages(
+        mut self,
+        target: &FunctionTarget,
+        eliminate_all_self_assigns: bool,
+    ) -> BTreeSet<u16> {
         let code = target.get_bytecode();
+        let cfg = StacklessControlFlowGraph::new_forward(code);
         let live_vars = target
             .get_annotations()
             .get::<LiveVarAnnotation>()
@@ -97,8 +107,19 @@ impl ReducedDefUseGraph {
         for dead_def_leaf in self.defs_dead.clone() {
             self.disconnect_from_parents(dead_def_leaf);
         }
-        // Stage 3: Let's disconnect all the self-assignments from the graph and kill them.
+        // Stage 3: Let's disconnect self-assignments from the graph and kill them
+        // (conditioned upon `eliminate_all_self_assigns`).
         for self_assign in self_assigns {
+            let eliminate_this_self_assign = Self::should_eliminate_given_self_assign(
+                self_assign,
+                code,
+                &cfg,
+                live_vars,
+                eliminate_all_self_assigns,
+            );
+            if !eliminate_this_self_assign {
+                continue;
+            }
             let mut parents = self.disconnect_from_parents(self_assign);
             let mut children = self.disconnect_from_children(self_assign);
             // In case there is a cycle of self-assignments in the graph.
@@ -208,12 +229,55 @@ impl ReducedDefUseGraph {
             self.defs_dead.insert(def); // def without a use is dead
         }
     }
+
+    /// Should `self_assign` be eliminated?
+    fn should_eliminate_given_self_assign(
+        self_assign: CodeOffset,
+        code: &[Bytecode],
+        cfg: &StacklessControlFlowGraph,
+        live_vars: &LiveVarAnnotation,
+        eliminate_all_self_assigns: bool,
+    ) -> bool {
+        if !eliminate_all_self_assigns {
+            // Eliminate this self assign if the definition for this self-assign is in the same block
+            // before the self assign.
+            let block = cfg.enclosing_block(self_assign);
+            let block_begin_offset = cfg.code_range(block).start;
+            let self_assign_instr = &code[self_assign as usize];
+            let self_assign_temp = self_assign_instr.dests()[0];
+            // Is `self_assign_temp` live before this block?
+            let info = live_vars
+                .get_info_at(block_begin_offset as CodeOffset)
+                .before
+                .get(&self_assign_temp);
+            match info {
+                None => true, // must be defined in the block
+                Some(live) => !live.usage_offsets().contains(&self_assign),
+            }
+        } else {
+            true
+        }
+    }
 }
 
 /// A processor which performs dead store elimination transformation.
-pub struct DeadStoreElimination {}
+pub struct DeadStoreElimination {
+    /// If true, eliminate all self-assignments of the form `x = x`.
+    /// Otherwise, only self assignments where the definition is in the same block
+    /// before the self-assign are removed.
+    eliminate_all_self_assigns: bool,
+}
 
 impl DeadStoreElimination {
+    /// If `eliminate_all_self_assigns` is true, all self-assignments are removed.
+    /// Otherwise, only self assignments where the definition is in the same block
+    /// before the self-assign are removed.
+    pub fn new(eliminate_all_self_assigns: bool) -> Self {
+        Self {
+            eliminate_all_self_assigns,
+        }
+    }
+
     /// Transforms the `code` of a function by removing the instructions corresponding to
     /// the code offsets contained in `dead_stores`.
     ///
@@ -242,7 +306,7 @@ impl FunctionTargetProcessor for DeadStoreElimination {
             return data;
         }
         let target = FunctionTarget::new(func_env, &data);
-        let dead_stores = ReducedDefUseGraph::dead_stores(&target);
+        let dead_stores = ReducedDefUseGraph::dead_stores(&target, self.eliminate_all_self_assigns);
         let new_code = Self::transform(&target, dead_stores);
         // Note that the file format generator will not include unused locals in the generated code,
         // so we don't need to prune unused locals here for various fields of `data` (like `local_types`).