Revert "Allow merging local variable even if empty." (#3361)

Author: orizi

crates/cairo-lang-sierra-to-casm/src/annotations.rs

@@ -5,7 +5,6 @@ use cairo_lang_casm::ap_change::{ApChangeError, ApplyApChange};
 use cairo_lang_sierra::edit_state::{put_results, take_args};
 use cairo_lang_sierra::ids::{FunctionId, VarId};
 use cairo_lang_sierra::program::{BranchInfo, Function, StatementIdx};
-use cairo_lang_utils::try_extract_matches;
 use cairo_lang_utils::unordered_hash_set::UnorderedHashSet;
 use itertools::zip_eq;
 use thiserror::Error;
@@ -22,7 +21,6 @@ use crate::metadata::Metadata;
 use crate::references::{
     build_function_parameters_refs, check_types_match, IntroductionPoint,
     OutputReferenceValueIntroductionPoint, ReferenceValue, ReferencesError, StatementRefs,
-    VariableApIndependentLocationInfo,
 };
 use crate::type_sizes::TypeSizeMap;
 
@@ -205,8 +203,7 @@ impl ProgramAnnotations {
             // Check if the variable doesn't match on type, expression or stack information.
             if !(actual_ref.ty == expected_ref.ty
                 && actual_ref.expression == expected_ref.expression
-                && actual_ref.ap_independent_location_info
-                    == expected_ref.ap_independent_location_info)
+                && actual_ref.stack_idx == expected_ref.stack_idx)
             {
                 return false;
             }
@@ -273,13 +270,10 @@ impl ProgramAnnotations {
                             error,
                         })?,
                     ty: ref_value.ty.clone(),
-                    ap_independent_location_info: match &ref_value.ap_independent_location_info {
-                        VariableApIndependentLocationInfo::ContinuousStack(_)
-                            if branch_changes.clear_old_stack =>
-                        {
-                            VariableApIndependentLocationInfo::Other
-                        }
-                        other => other.clone(),
+                    stack_idx: if branch_changes.clear_old_stack {
+                        None
+                    } else {
+                        ref_value.stack_idx
                     },
                     introduction_point: ref_value.introduction_point.clone(),
                 },
@@ -292,7 +286,7 @@ impl ProgramAnnotations {
                 branch_changes.refs.into_iter().map(|value| ReferenceValue {
                     expression: value.expression,
                     ty: value.ty,
-                    ap_independent_location_info: value.ap_independent_location_info,
+                    stack_idx: value.stack_idx,
                     introduction_point: match value.introduction_point {
                         OutputReferenceValueIntroductionPoint::New(output_idx) => {
                             IntroductionPoint {
@@ -317,34 +311,18 @@ impl ProgramAnnotations {
         // Since some variables on the stack may have been consumed by the libfunc, we need to
         // find the new stack size. This is done by searching from the bottom of the stack until we
         // find a missing variable.
-        let available_stack_indices: UnorderedHashSet<_> = refs
-            .values()
-            .flat_map(|r| {
-                try_extract_matches!(
-                    r.ap_independent_location_info,
-                    VariableApIndependentLocationInfo::ContinuousStack
-                )
-            })
-            .collect();
+        let available_stack_indices: UnorderedHashSet<_> =
+            refs.values().flat_map(|r| r.stack_idx).collect();
         let new_stack_size_opt = (0..branch_changes.new_stack_size)
             .find(|i| !available_stack_indices.contains(&(branch_changes.new_stack_size - 1 - i)));
         let stack_size = if let Some(new_stack_size) = new_stack_size_opt {
             // The number of stack elements which were removed.
             let stack_removal = branch_changes.new_stack_size - new_stack_size;
             for r in refs.values_mut() {
-                r.ap_independent_location_info = match &r.ap_independent_location_info {
-                    VariableApIndependentLocationInfo::ContinuousStack(stack_idx) => {
-                        // Subtract the number of stack elements removed. If the result is negative,
-                        // `stack_idx` is set to `None` and the variable is removed from the stack.
-                        match stack_idx.checked_sub(stack_removal) {
-                            Some(new_stack_idx) => {
-                                VariableApIndependentLocationInfo::ContinuousStack(new_stack_idx)
-                            }
-                            None => VariableApIndependentLocationInfo::Other,
-                        }
-                    }
-                    other => other.clone(),
-                };
+                // Subtract the number of stack elements removed. If the result is negative,
+                // `stack_idx` is set to `None` and the variable is removed from the stack.
+                r.stack_idx =
+                    r.stack_idx.and_then(|stack_idx| stack_idx.checked_sub(stack_removal));
             }
             new_stack_size
         } else {
@@ -456,12 +434,8 @@ fn test_var_consistency(
     expected: &ReferenceValue,
     ap_tracking_enabled: bool,
 ) -> bool {
-    // If the variable is on the stack, or is local, it can always be merged.
-    if matches!(
-        actual.ap_independent_location_info,
-        VariableApIndependentLocationInfo::ContinuousStack(_)
-            | VariableApIndependentLocationInfo::Local
-    ) {
+    // If the variable is on the stack, it can always be merged.
+    if actual.stack_idx.is_some() {
         return true;
     }
     // If the variable is not ap-dependent it can always be merged.

crates/cairo-lang-sierra-to-casm/src/invocations/mod.rs

@@ -25,7 +25,7 @@ use crate::environment::Environment;
 use crate::metadata::Metadata;
 use crate::references::{
     OutputReferenceValue, OutputReferenceValueIntroductionPoint, ReferenceExpression,
-    ReferenceValue, VariableApIndependentLocationInfo,
+    ReferenceValue,
 };
 use crate::relocations::{InstructionsWithRelocations, Relocation, RelocationEntry};
 use crate::type_sizes::TypeSizeMap;
@@ -136,15 +136,13 @@ impl BranchChanges {
                 .enumerate()
                 .map(|(output_idx, (expression, OutputVarInfo { ref_info, ty }))| {
                     validate_output_var_refs(ref_info, &expression);
-                    let ap_independent_location_info = calc_output_ap_independent_location_info(
+                    let stack_idx = calc_output_var_stack_idx(
                         ref_info,
                         stack_base,
                         clear_old_stack,
                         &param_ref,
                     );
-                    if let VariableApIndependentLocationInfo::ContinuousStack(stack_idx) =
-                        ap_independent_location_info
-                    {
+                    if let Some(stack_idx) = stack_idx {
                         new_stack_size = new_stack_size.max(stack_idx + 1);
                     }
                     let introduction_point =
@@ -159,7 +157,7 @@ impl BranchChanges {
                     OutputReferenceValue {
                         expression,
                         ty: ty.clone(),
-                        ap_independent_location_info,
+                        stack_idx,
                         introduction_point,
                     }
                 })
@@ -200,24 +198,22 @@ fn validate_output_var_refs(ref_info: &OutputVarReferenceInfo, expression: &Refe
 
 /// Calculates the continuous stack index for an output var of a branch.
 /// `param_ref` is used to fetch the reference value of a param of the libfunc.
-fn calc_output_ap_independent_location_info<'a, ParamRef: Fn(usize) -> &'a ReferenceValue>(
+fn calc_output_var_stack_idx<'a, ParamRef: Fn(usize) -> &'a ReferenceValue>(
     ref_info: &OutputVarReferenceInfo,
     stack_base: usize,
     clear_old_stack: bool,
     param_ref: &ParamRef,
-) -> VariableApIndependentLocationInfo {
+) -> Option<usize> {
     match ref_info {
-        OutputVarReferenceInfo::NewTempVar { idx } => {
-            VariableApIndependentLocationInfo::ContinuousStack(stack_base + idx)
-        }
-        OutputVarReferenceInfo::NewLocalVar => VariableApIndependentLocationInfo::Local,
+        OutputVarReferenceInfo::NewTempVar { idx } => Some(stack_base + idx),
         OutputVarReferenceInfo::SameAsParam { param_idx } if !clear_old_stack => {
-            param_ref(*param_idx).ap_independent_location_info.clone()
+            param_ref(*param_idx).stack_idx
         }
         OutputVarReferenceInfo::SameAsParam { .. }
         | OutputVarReferenceInfo::SimpleDerefs
+        | OutputVarReferenceInfo::NewLocalVar
         | OutputVarReferenceInfo::PartialParam { .. }
-        | OutputVarReferenceInfo::Deferred(_) => VariableApIndependentLocationInfo::Other,
+        | OutputVarReferenceInfo::Deferred(_) => None,
     }
 }
 

crates/cairo-lang-sierra-to-casm/src/invocations/test_utils.rs

@@ -21,9 +21,7 @@ use super::{compile_invocation, CompiledInvocation, ProgramInfo};
 use crate::environment::gas_wallet::GasWallet;
 use crate::environment::Environment;
 use crate::metadata::Metadata;
-use crate::references::{
-    IntroductionPoint, ReferenceExpression, ReferenceValue, VariableApIndependentLocationInfo,
-};
+use crate::references::{IntroductionPoint, ReferenceExpression, ReferenceValue};
 use crate::relocations::RelocationEntry;
 
 /// Creates a Felt252BinaryOperator from a token operator.
@@ -269,7 +267,7 @@ pub fn compile_libfunc(libfunc: &str, refs: Vec<ReferenceExpression>) -> Reduced
         .map(|(expression, param)| ReferenceValue {
             expression,
             ty: param.ty.clone(),
-            ap_independent_location_info: VariableApIndependentLocationInfo::Other,
+            stack_idx: None,
             introduction_point: IntroductionPoint {
                 source_statement_idx: None,
                 destination_statement_idx: StatementIdx(0),

crates/cairo-lang-sierra-to-casm/src/references.rs

@@ -26,28 +26,15 @@ pub enum ReferencesError {
 
 pub type StatementRefs = HashMap<VarId, ReferenceValue>;
 
-/// Information about the location of a variable that is not dependent on the actual AP values.
-/// Used for recompilability checks.
-#[derive(Clone, Debug, Eq, PartialEq)]
-pub enum VariableApIndependentLocationInfo {
-    /// The variable is on the continuous-stack, with the given index. 0 represents the oldest
-    /// element on the stack.
-    ContinuousStack(usize),
-    /// The variable was defined as a local variable. May be a function param, or an explicitly
-    /// defined local.
-    Local,
-    /// Any other case.
-    Other,
-}
-
 /// A Sierra reference to a value.
 /// Corresponds to an argument or return value of a Sierra statement.
 #[derive(Clone, Debug)]
 pub struct ReferenceValue {
     pub expression: ReferenceExpression,
     pub ty: ConcreteTypeId,
-    /// Location information for the variable that does not depend on AP.
-    pub ap_independent_location_info: VariableApIndependentLocationInfo,
+    /// The index of the variable on the continuous-stack. 0 represents the oldest element on the
+    /// stack.
+    pub stack_idx: Option<usize>,
     /// The location the value was introduced.
     pub introduction_point: IntroductionPoint,
 }
@@ -78,8 +65,8 @@ pub struct IntroductionPoint {
 pub struct OutputReferenceValue {
     pub expression: ReferenceExpression,
     pub ty: ConcreteTypeId,
-    /// Location information for the variable that does not depend on AP.
-    pub ap_independent_location_info: VariableApIndependentLocationInfo,
+    /// The index of the variable on the continuous-stack.
+    pub stack_idx: Option<usize>,
     /// The statememt and output index where the value was introduced.
     /// Statement may be New if it is to be populated later.
     pub introduction_point: OutputReferenceValueIntroductionPoint,
@@ -159,7 +146,7 @@ pub fn build_function_parameters_refs(
                             .collect(),
                     },
                     ty: param.ty.clone(),
-                    ap_independent_location_info: VariableApIndependentLocationInfo::Local,
+                    stack_idx: None,
                     introduction_point: IntroductionPoint {
                         source_statement_idx: None,
                         destination_statement_idx: func.entry_point,

tests/bug_samples/issue3345.cairo

@@ -19,7 +19,6 @@ mod issue2995;
 mod issue3153;
 mod issue3192;
 mod issue3211;
-mod issue3345;
 mod loop_only_change;
 mod inconsistent_gas;
 mod partial_param_local;