[move-compiler-v2] Implement AST Simplification to solve aptos-labs#10850 (aptos-labs#11553)

This is an expression simplifier which works at the AST level in Compiler V2,
doing some quick optimizations that reduce the amount of code the stackless
bytecode passes have to deal with. Notably, it eliminates some of the overhead
of inlining, as seen in the test case deep_exp.move.

* Fixes aptos-labs#10850: Compiler V2 needs expression simplification
* Fixes aptos-labs#11535: Constant folding should not allow full-bitsize shifts
* Fixes aptos-labs#11536: Constant folding displays redundant errors
* Fixes aptos-labs#11549: Model Dump for a Cast operation doesn't show the type
* Fixes aptos-labs#11805: Load constants the right way
* Fixes aptos-labs#11720: Process got killed when running unit tests in aptos-gas-schedule-updator using V2

Author: brmataptos

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

@@ -254,15 +254,21 @@ impl<'env> Generator<'env> {
             self.label_counter += 1;
             Label::new(n as usize)
         } else {
-            self.internal_error(id, "too many labels");
+            self.internal_error(id, format!("too many labels: {}", self.label_counter));
             Label::new(0)
         }
     }
 
     /// Require unary target.
     fn require_unary_target(&mut self, id: NodeId, target: Vec<TempIndex>) -> TempIndex {
         if target.len() != 1 {
-            self.internal_error(id, "inconsistent expression target arity");
+            self.internal_error(
+                id,
+                format!(
+                    "inconsistent expression target arity: {} and 1",
+                    target.len()
+                ),
+            );
             0
         } else {
             target[0]
@@ -273,7 +279,13 @@ impl<'env> Generator<'env> {
     /// interning.
     fn require_unary_arg(&self, id: NodeId, args: &[Exp]) -> Exp {
         if args.len() != 1 {
-            self.internal_error(id, "inconsistent expression argument arity");
+            self.internal_error(
+                id,
+                format!(
+                    "inconsistent expression argument arity: {} and 1",
+                    args.len()
+                ),
+            );
             ExpData::Invalid(self.env().new_node_id()).into_exp()
         } else {
             args[0].to_owned()
@@ -501,6 +513,18 @@ impl<'env> Generator<'env> {
             Value::Bool(x) => Constant::Bool(*x),
             Value::ByteArray(x) => Constant::ByteArray(x.clone()),
             Value::AddressArray(x) => Constant::AddressArray(x.clone()),
+            Value::Tuple(x) => {
+                if let Some(inner_ty) = ty.get_vector_element_type() {
+                    Constant::Vector(
+                        x.iter()
+                            .map(|v| self.to_constant(id, inner_ty.clone(), v))
+                            .collect(),
+                    )
+                } else {
+                    self.internal_error(id, format!("inconsistent tuple type: {:?}", ty));
+                    Constant::Bool(false)
+                }
+            },
             Value::Vector(x) => {
                 if let Some(inner_ty) = ty.get_vector_element_type() {
                     Constant::Vector(
@@ -546,7 +570,14 @@ impl<'env> Generator<'env> {
             Operation::Freeze => self.gen_op_call(targets, id, BytecodeOperation::FreezeRef, args),
             Operation::Tuple => {
                 if targets.len() != args.len() {
-                    self.internal_error(id, "inconsistent tuple arity")
+                    self.internal_error(
+                        id,
+                        format!(
+                            "inconsistent tuple arity: {} and {}",
+                            targets.len(),
+                            args.len()
+                        ),
+                    )
                 } else {
                     for (target, arg) in targets.into_iter().zip(args.iter()) {
                         self.gen(vec![target], arg)
@@ -1216,6 +1247,7 @@ impl<'env> Generator<'env> {
         match pat {
             Pattern::Wildcard(_) => {
                 // Nothing to do
+                // TODO(#12475) Should we copy to a temp here?
             },
             Pattern::Var(var_id, sym) => {
                 let local = self.find_local_for_pattern(*var_id, *sym, next_scope);

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

@@ -223,12 +223,16 @@ impl<'a> ExpRewriterFunctions for LambdaLifter<'a> {
         }
     }
 
-    fn rewrite_enter_scope<'b>(&mut self, vars: impl Iterator<Item = &'b (NodeId, Symbol)>) {
+    fn rewrite_enter_scope<'b>(
+        &mut self,
+        _id: NodeId,
+        vars: impl Iterator<Item = &'b (NodeId, Symbol)>,
+    ) {
         self.scopes
             .push(vars.map(|(_, sym)| sym).cloned().collect())
     }
 
-    fn rewrite_exit_scope(&mut self) {
+    fn rewrite_exit_scope(&mut self, _id: NodeId) {
         let exiting = self.scopes.pop().expect("stack balanced");
         // Remove all locals which are bound in the scope we are exiting.
         self.free_locals.retain(|name, _| !exiting.contains(name));

third_party/move/move-compiler-v2/src/env_pipeline/lambda_lifter.rs

@@ -8,7 +8,7 @@
 
 use log::debug;
 use move_model::model::GlobalEnv;
-use std::fmt::Write;
+use std::io::Write;
 
 pub mod lambda_lifter;
 

third_party/move/move-compiler-v2/src/env_pipeline/mod.rs

@@ -112,7 +112,7 @@ impl<'env, 'params> SymbolVisitor<'env, 'params> {
                         }
                         self.seen_uses.exit_scope();
                     },
-                    Pre | MidMutate | BeforeThen | BeforeElse => {},
+                    Pre | MidMutate | BeforeThen | BeforeElse | PreSequenceValue => {},
                 };
             },
             Lambda(_, pat, _) => {
@@ -129,7 +129,7 @@ impl<'env, 'params> SymbolVisitor<'env, 'params> {
                         }
                         self.seen_uses.exit_scope();
                     },
-                    BeforeBody | MidMutate | BeforeThen | BeforeElse => {},
+                    BeforeBody | MidMutate | BeforeThen | BeforeElse | PreSequenceValue => {},
                 };
             },
             Quant(_, _, ranges, ..) => {
@@ -141,7 +141,7 @@ impl<'env, 'params> SymbolVisitor<'env, 'params> {
                         }
                         self.seen_uses.exit_scope();
                     },
-                    BeforeBody | MidMutate | BeforeThen | BeforeElse => {},
+                    BeforeBody | MidMutate | BeforeThen | BeforeElse | PreSequenceValue => {},
                 };
             },
             Assign(_, pat, _) => {

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

@@ -36,10 +36,9 @@
 ///
 /// - TODO(10858): add an anchor AST node so we can implement `Return` for inline functions and
 ///   `Lambda`.
-/// - TODO(10850): add a simplifier that simplifies certain code constructs.
 use codespan_reporting::diagnostic::Severity;
 use itertools::chain;
-use log::{info, trace};
+use log::{debug, trace};
 use move_model::{
     ast::{Exp, ExpData, Operation, Pattern, TempIndex},
     exp_rewriter::ExpRewriterFunctions,
@@ -64,7 +63,7 @@ type CallSiteLocations = BTreeMap<(QualifiedFunId, QualifiedFunId), BTreeSet<Nod
 /// Run inlining on current program's AST.  For each function which is target of the compilation,
 /// visit that function body and inline any calls to functions marked as "inline".
 pub fn run_inlining(env: &mut GlobalEnv) {
-    info!("Inlining");
+    debug!("Inlining");
     // Get non-inline function roots for running inlining.
     // Also generate an error for any target inline functions lacking a body to inline.
     let mut todo = get_targets(env);
@@ -1026,15 +1025,19 @@ impl<'env, 'rewriter> ExpRewriterFunctions for InlinedRewriter<'env, 'rewriter>
 
     /// Record that the provided symbols have local definitions, so renaming should be done.
     /// Note that incoming vars are from a Pattern *after* renaming, so these are shadowed symbols.
-    fn rewrite_enter_scope<'a>(&mut self, vars: impl Iterator<Item = &'a (NodeId, Symbol)>) {
+    fn rewrite_enter_scope<'a>(
+        &mut self,
+        _id: NodeId,
+        vars: impl Iterator<Item = &'a (NodeId, Symbol)>,
+    ) {
         self.shadow_stack
             .enter_scope_after_renaming(vars.map(|(_, sym)| sym));
     }
 
     /// On exiting a scope defining some symbols shadowing lambda free vars, record that we have
     /// exited the scope so any occurrences of those free vars should be left alone (if there are
     /// not further shadowing scopes further out).
-    fn rewrite_exit_scope(&mut self) {
+    fn rewrite_exit_scope(&mut self, _id: NodeId) {
         self.shadow_stack.exit_scope();
     }
 

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

@@ -2,21 +2,36 @@
 // Parts of the project are originally copyright © Meta Platforms, Inc.
 // SPDX-License-Identifier: Apache-2.0
 
-use crate::pipeline::{
-    ability_processor::AbilityProcessor, avail_copies_analysis::AvailCopiesAnalysisProcessor,
-    copy_propagation::CopyPropagation, dead_store_elimination::DeadStoreElimination,
-    exit_state_analysis::ExitStateAnalysisProcessor,
-    livevar_analysis_processor::LiveVarAnalysisProcessor,
-    reference_safety_processor::ReferenceSafetyProcessor,
-    split_critical_edges_processor::SplitCriticalEdgesProcessor,
-    uninitialized_use_checker::UninitializedUseChecker,
-    unreachable_code_analysis::UnreachableCodeProcessor,
-    unreachable_code_remover::UnreachableCodeRemover, variable_coalescing::VariableCoalescing,
+pub mod ast_simplifier;
+mod bytecode_generator;
+pub mod env_pipeline;
+mod experiments;
+mod file_format_generator;
+pub mod flow_insensitive_checkers;
+pub mod function_checker;
+pub mod inliner;
+pub mod logging;
+pub mod options;
+pub mod pipeline;
+
+use crate::{
+    env_pipeline::EnvProcessorPipeline,
+    pipeline::{
+        ability_processor::AbilityProcessor, avail_copies_analysis::AvailCopiesAnalysisProcessor,
+        copy_propagation::CopyPropagation, dead_store_elimination::DeadStoreElimination,
+        exit_state_analysis::ExitStateAnalysisProcessor,
+        livevar_analysis_processor::LiveVarAnalysisProcessor,
+        reference_safety_processor::ReferenceSafetyProcessor,
+        split_critical_edges_processor::SplitCriticalEdgesProcessor,
+        uninitialized_use_checker::UninitializedUseChecker,
+        unreachable_code_analysis::UnreachableCodeProcessor,
+        unreachable_code_remover::UnreachableCodeRemover, variable_coalescing::VariableCoalescing,
+    },
 };
 use anyhow::bail;
 use codespan_reporting::term::termcolor::{ColorChoice, StandardStream, WriteColor};
 pub use experiments::*;
-use log::{debug, info, log_enabled, trace, Level};
+use log::{debug, info, log_enabled, Level};
 use move_binary_format::binary_views::BinaryIndexedView;
 use move_command_line_common::files::FileHash;
 use move_compiler::{
@@ -35,18 +50,7 @@ use move_stackless_bytecode::function_target_pipeline::{
 };
 use move_symbol_pool::Symbol;
 pub use options::*;
-use std::{collections::BTreeSet, path::Path};
-
-mod bytecode_generator;
-pub mod env_pipeline;
-mod experiments;
-mod file_format_generator;
-pub mod flow_insensitive_checkers;
-pub mod function_checker;
-pub mod inliner;
-pub mod logging;
-pub mod options;
-pub mod pipeline;
+use std::{collections::BTreeSet, io::Write, path::Path};
 
 /// Run Move compiler and print errors to stderr.
 pub fn run_move_compiler_to_stderr(
@@ -57,41 +61,38 @@ pub fn run_move_compiler_to_stderr(
 }
 
 /// Run move compiler and print errors to given writer.
-pub fn run_move_compiler(
-    error_writer: &mut impl WriteColor,
+pub fn run_move_compiler<W>(
+    error_writer: &mut W,
     options: Options,
-) -> anyhow::Result<(GlobalEnv, Vec<AnnotatedCompiledUnit>)> {
+) -> anyhow::Result<(GlobalEnv, Vec<AnnotatedCompiledUnit>)>
+where
+    W: WriteColor + Write,
+{
     logging::setup_logging();
     info!("Move Compiler v2");
     // Run context check.
     let mut env = run_checker(options.clone())?;
     check_errors(&env, error_writer, "checking errors")?;
 
-    trace!("After context check, GlobalEnv=\n{}", env.dump_env());
+    debug!("After context check, GlobalEnv=\n{}", env.dump_env());
 
-    // Flow-insensitive checks on AST
-    flow_insensitive_checkers::check_for_unused_vars_and_params(&mut env);
-    function_checker::check_for_function_typed_parameters(&mut env);
-    function_checker::check_access_and_use(&mut env, true);
+    let env_pipeline = create_env_processor_pipeline(&env);
+    if log_enabled!(Level::Debug) {
+        env_pipeline.run_and_record(&mut env, error_writer)?;
+    } else {
+        env_pipeline.run(&mut env);
+    }
     check_errors(&env, error_writer, "checking errors")?;
 
-    trace!(
+    debug!(
         "After flow-insensitive checks, GlobalEnv=\n{}",
         env.dump_env()
     );
 
-    // Run inlining.
-    inliner::run_inlining(&mut env);
-    check_errors(&env, error_writer, "inlining")?;
-
-    debug!("After inlining, GlobalEnv=\n{}", env.dump_env());
-
-    function_checker::check_access_and_use(&mut env, false);
-    check_errors(&env, error_writer, "post-inlining access checks")?;
-
     // Run code generator
     let mut targets = run_bytecode_gen(&env);
     check_errors(&env, error_writer, "code generation errors")?;
+    debug!("After bytecode_gen, GlobalEnv={}", env.dump_env());
 
     // Run transformation pipeline
     let pipeline = bytecode_pipeline(&env);
@@ -204,6 +205,40 @@ pub fn run_file_format_gen(env: &GlobalEnv, targets: &FunctionTargetsHolder) ->
     file_format_generator::generate_file_format(env, targets)
 }
 
+/// Returns the standard env_processor_pipeline
+pub fn create_env_processor_pipeline<'b>(env: &GlobalEnv) -> EnvProcessorPipeline<'b> {
+    let options = env.get_extension::<Options>().expect("options");
+    let optimize_on = options.experiment_on(Experiment::OPTIMIZE);
+
+    let mut env_pipeline = EnvProcessorPipeline::default();
+    env_pipeline.add(
+        "unused vars and params checks",
+        flow_insensitive_checkers::check_for_unused_vars_and_params,
+    );
+    env_pipeline.add(
+        "function typed parameter check",
+        function_checker::check_for_function_typed_parameters,
+    );
+    env_pipeline.add(
+        "access and use check before inlining",
+        |env: &mut GlobalEnv| function_checker::check_access_and_use(env, true),
+    );
+    env_pipeline.add("inlining", inliner::run_inlining);
+    env_pipeline.add(
+        "access and use check after inlining",
+        |env: &mut GlobalEnv| function_checker::check_access_and_use(env, false),
+    );
+    env_pipeline.add("simplifier", {
+        move |env: &mut GlobalEnv| {
+            ast_simplifier::run_simplifier(
+                env,
+                optimize_on, // eliminate code only if optimize is on
+            )
+        }
+    });
+    env_pipeline
+}
+
 /// Returns the bytecode processing pipeline.
 pub fn bytecode_pipeline(env: &GlobalEnv) -> FunctionTargetPipeline {
     let options = env.get_extension::<Options>().expect("options");
@@ -274,11 +309,14 @@ pub fn run_bytecode_verifier(units: &[AnnotatedCompiledUnit], env: &mut GlobalEn
 }
 
 /// Report any diags in the env to the writer and fail if there are errors.
-pub fn check_errors<W: WriteColor>(
+pub fn check_errors<W>(
     env: &GlobalEnv,
     error_writer: &mut W,
     msg: &'static str,
-) -> anyhow::Result<()> {
+) -> anyhow::Result<()>
+where
+    W: WriteColor + Write,
+{
     let options = env.get_extension::<Options>().unwrap_or_default();
     env.report_diag(error_writer, options.report_severity());
     if env.has_errors() {

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

@@ -1322,8 +1322,8 @@ impl<'env, 'state> LifetimeAnalysisStep<'env, 'state> {
                     infos.push((
                         loc.clone(),
                         format!(
-                            "conflicting reference {}used here",
-                            self.display_name_or_empty("", temp)
+                            "conflicting reference{} used here",
+                            self.display_name_or_empty(" ", temp)
                         ),
                     ))
                 }

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

@@ -16,7 +16,7 @@ module 0x42::assign {
         Tuple()
     }
     private fun assign_pattern(s: assign::S,f: u64,h: u64): u64 {
-        assign::S{ f: f: u64, g: assign::T{ h: h: u64 } } = s;
+        assign::S{ f: f: u64, g: assign::T{ h: h: u64 } }: assign::S = s;
         Add<u64>(f, h)
     }
     private fun assign_struct(s: &mut assign::S) {