[1 changes] fix(mem2reg): Handle aliases in function last store cleanup and additional alias unit test (noir-lang/noir#5967)

fix: let `derive(Eq)` work for empty structs (noir-lang/noir#5965)
feat: add `FunctionDefinition` methods `is_unconstrained` and `set_unconstrained` (noir-lang/noir#5962)
feat: LSP autocompletion for attributes (noir-lang/noir#5963)
feat: `Module::add_item` (noir-lang/noir#5947)
feat: Add `StructDefinition::add_generic` (noir-lang/noir#5961)
feat: Add `StructDefinition::name` (noir-lang/noir#5960)
fix(mem2reg): Handle aliases better when setting a known value for a load (noir-lang/noir#5959)
feat: Arithmetic Generics (noir-lang/noir#5950)
feat: add `FunctionDefinition::module` and `StructDefinition::module` (noir-lang/noir#5956)
feat: LSP now suggests self fields and methods (noir-lang/noir#5955)

Author: AztecBot

.noir-sync-commit

@@ -1 +1 @@
-b84009ca428a5790acf53a6c027146b706170574
+36756e8757ad40e2b231747ed754273f50e5dc2f

noir/noir-repo/.gitattributes

@@ -25,7 +25,7 @@ function run_if_available {
 require_command jq
 require_command cargo
 require_command wasm-bindgen
-#require_command wasm-opt
+require_command wasm-opt
 
 self_path=$(dirname "$(readlink -f "$0")")
 pname=$(cargo read-manifest | jq -r '.name')

noir/noir-repo/acvm-repo/acvm_js/build.sh

@@ -195,7 +195,7 @@ pub fn inject_fn(
     let trait_id = None;
     items.functions.push(UnresolvedFunctions { file_id, functions, trait_id, self_type: None });
 
-    let mut errors = Elaborator::elaborate(context, *crate_id, items, None, false);
+    let mut errors = Elaborator::elaborate(context, *crate_id, items, None);
     errors.retain(|(error, _)| !CustomDiagnostic::from(error).is_warning());
 
     if !errors.is_empty() {
@@ -241,7 +241,7 @@ pub fn inject_global(
     let mut items = CollectedItems::default();
     items.globals.push(UnresolvedGlobal { file_id, module_id, global_id, stmt_def: global });
 
-    let _errors = Elaborator::elaborate(context, *crate_id, items, None, false);
+    let _errors = Elaborator::elaborate(context, *crate_id, items, None);
 }
 
 pub fn fully_qualified_note_path(context: &HirContext, note_id: StructId) -> Option<String> {

noir/noir-repo/aztec_macros/src/utils/hir_utils.rs

@@ -120,10 +120,6 @@ pub struct CompileOptions {
     #[arg(long, hide = true)]
     pub show_artifact_paths: bool,
 
-    /// Temporary flag to enable the experimental arithmetic generics feature
-    #[arg(long, hide = true)]
-    pub arithmetic_generics: bool,
-
     /// Flag to turn off the compiler check for under constrained values.
     /// Warning: This can improve compilation speed but can also lead to correctness errors.
     /// This check should always be run on production code.
@@ -289,7 +285,6 @@ pub fn check_crate(
         crate_id,
         context,
         options.debug_comptime_in_file.as_deref(),
-        options.arithmetic_generics,
         error_on_unused_imports,
         macros,
     );

noir/noir-repo/compiler/noirc_driver/src/lib.rs

@@ -181,7 +181,12 @@ impl<'f> PerFunctionContext<'f> {
                     self.last_loads.get(store_address).is_none()
                 };
 
-                if remove_load && !is_reference_param {
+                let is_reference_alias = block
+                    .expressions
+                    .get(store_address)
+                    .map_or(false, |expression| matches!(expression, Expression::Dereference(_)));
+
+                if remove_load && !is_reference_param && !is_reference_alias {
                     self.instructions_to_remove.insert(*store_instruction);
                 }
             }
@@ -286,19 +291,19 @@ impl<'f> PerFunctionContext<'f> {
                 } else {
                     references.mark_value_used(address, self.inserter.function);
 
-                    let expression = if let Some(expression) = references.expressions.get(&result) {
-                        expression.clone()
-                    } else {
-                        references.expressions.insert(result, Expression::Other(result));
-                        Expression::Other(result)
-                    };
-                    if let Some(aliases) = references.aliases.get_mut(&expression) {
+                    let expression =
+                        references.expressions.entry(result).or_insert(Expression::Other(result));
+                    // Make sure this load result is marked an alias to itself
+                    if let Some(aliases) = references.aliases.get_mut(expression) {
+                        // If we have an alias set, add to the set
                         aliases.insert(result);
                     } else {
+                        // Otherwise, create a new alias set containing just the load result
                         references
                             .aliases
                             .insert(Expression::Other(result), AliasSet::known(result));
                     }
+                    // Mark that we know a load result is equivalent to the address of a load.
                     references.set_known_value(result, address);
 
                     self.last_loads.insert(address, (instruction, block_id));
@@ -789,4 +794,98 @@ mod tests {
         // We expect the last eq to be optimized out
         assert_eq!(b1_instructions.len(), 0);
     }
+
+    #[test]
+    fn keep_store_to_alias_in_loop_block() {
+        // This test makes sure the instruction `store Field 2 at v5` in b2 remains after mem2reg.
+        // Although the only instruction on v5 is a lone store without any loads,
+        // v5 is an alias of the reference v0 which is stored in v2.
+        // This test makes sure that we are not inadvertently removing stores to aliases across blocks.
+        //
+        // acir(inline) fn main f0 {
+        //     b0():
+        //       v0 = allocate
+        //       store Field 0 at v0
+        //       v2 = allocate
+        //       store v0 at v2
+        //       jmp b1(Field 0)
+        //     b1(v3: Field):
+        //       v4 = eq v3, Field 0
+        //       jmpif v4 then: b2, else: b3
+        //     b2():
+        //       v5 = load v2
+        //       store Field 2 at v5
+        //       v8 = add v3, Field 1
+        //       jmp b1(v8)
+        //     b3():
+        //       v9 = load v0
+        //       v10 = eq v9, Field 2
+        //       constrain v9 == Field 2
+        //       v11 = load v2
+        //       v12 = load v10
+        //       v13 = eq v12, Field 2
+        //       constrain v11 == Field 2
+        //       return
+        //   }
+        let main_id = Id::test_new(0);
+        let mut builder = FunctionBuilder::new("main".into(), main_id);
+
+        let v0 = builder.insert_allocate(Type::field());
+        let zero = builder.numeric_constant(0u128, Type::field());
+        builder.insert_store(v0, zero);
+
+        let v2 = builder.insert_allocate(Type::field());
+        // Construct alias
+        builder.insert_store(v2, v0);
+        let v2_type = builder.current_function.dfg.type_of_value(v2);
+        assert!(builder.current_function.dfg.value_is_reference(v2));
+
+        let b1 = builder.insert_block();
+        builder.terminate_with_jmp(b1, vec![zero]);
+
+        // Loop header
+        builder.switch_to_block(b1);
+        let v3 = builder.add_block_parameter(b1, Type::field());
+        let is_zero = builder.insert_binary(v3, BinaryOp::Eq, zero);
+
+        let b2 = builder.insert_block();
+        let b3 = builder.insert_block();
+        builder.terminate_with_jmpif(is_zero, b2, b3);
+
+        // Loop body
+        builder.switch_to_block(b2);
+        let v5 = builder.insert_load(v2, v2_type.clone());
+        let two = builder.numeric_constant(2u128, Type::field());
+        builder.insert_store(v5, two);
+        let one = builder.numeric_constant(1u128, Type::field());
+        let v3_plus_one = builder.insert_binary(v3, BinaryOp::Add, one);
+        builder.terminate_with_jmp(b1, vec![v3_plus_one]);
+
+        builder.switch_to_block(b3);
+        let v9 = builder.insert_load(v0, Type::field());
+        let _ = builder.insert_binary(v9, BinaryOp::Eq, two);
+
+        builder.insert_constrain(v9, two, None);
+        let v11 = builder.insert_load(v2, v2_type);
+        let v12 = builder.insert_load(v11, Type::field());
+        let _ = builder.insert_binary(v12, BinaryOp::Eq, two);
+
+        builder.insert_constrain(v11, two, None);
+        builder.terminate_with_return(vec![]);
+
+        let ssa = builder.finish();
+
+        // We expect the same result as above.
+        let ssa = ssa.mem2reg();
+
+        let main = ssa.main();
+        assert_eq!(main.reachable_blocks().len(), 4);
+
+        // The store from the original SSA should remain
+        assert_eq!(count_stores(main.entry_block(), &main.dfg), 2);
+        assert_eq!(count_stores(b2, &main.dfg), 1);
+
+        assert_eq!(count_loads(b2, &main.dfg), 1);
+        assert_eq!(count_loads(b3, &main.dfg), 3);
+    }
 }

noir/noir-repo/compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs

@@ -12,6 +12,7 @@ mod traits;
 mod type_alias;
 mod visitor;
 
+pub use visitor::AttributeTarget;
 pub use visitor::Visitor;
 
 pub use expression::*;

noir/noir-repo/compiler/noirc_frontend/src/ast/mod.rs

@@ -16,7 +16,7 @@ use crate::{
         QuotedTypeId,
     },
     parser::{Item, ItemKind, ParsedSubModule},
-    token::{SecondaryAttribute, Tokens},
+    token::{CustomAtrribute, SecondaryAttribute, Tokens},
     ParsedModule, QuotedType,
 };
 
@@ -26,6 +26,13 @@ use super::{
     UnresolvedTypeExpression,
 };
 
+#[derive(Debug, Copy, Clone, PartialEq, Eq)]
+pub enum AttributeTarget {
+    Module,
+    Struct,
+    Function,
+}
+
 /// Implements the [Visitor pattern](https://en.wikipedia.org/wiki/Visitor_pattern) for Noir's AST.
 ///
 /// In this implementation, methods must return a bool:
@@ -433,7 +440,15 @@ pub trait Visitor {
         true
     }
 
-    fn visit_secondary_attribute(&mut self, _: &SecondaryAttribute, _: Span) {}
+    fn visit_secondary_attribute(
+        &mut self,
+        _: &SecondaryAttribute,
+        _target: AttributeTarget,
+    ) -> bool {
+        true
+    }
+
+    fn visit_custom_attribute(&mut self, _: &CustomAtrribute, _target: AttributeTarget) {}
 }
 
 impl ParsedModule {
@@ -484,14 +499,18 @@ impl Item {
                 module_declaration.accept(self.span, visitor);
             }
             ItemKind::InnerAttribute(attribute) => {
-                attribute.accept(self.span, visitor);
+                attribute.accept(AttributeTarget::Module, visitor);
             }
         }
     }
 }
 
 impl ParsedSubModule {
     pub fn accept(&self, span: Span, visitor: &mut impl Visitor) {
+        for attribute in &self.outer_attributes {
+            attribute.accept(AttributeTarget::Module, visitor);
+        }
+
         if visitor.visit_parsed_submodule(self, span) {
             self.accept_children(visitor);
         }
@@ -510,6 +529,10 @@ impl NoirFunction {
     }
 
     pub fn accept_children(&self, visitor: &mut impl Visitor) {
+        for attribute in self.secondary_attributes() {
+            attribute.accept(AttributeTarget::Function, visitor);
+        }
+
         for param in &self.def.parameters {
             param.typ.accept(visitor);
         }
@@ -674,6 +697,10 @@ impl NoirStruct {
     }
 
     pub fn accept_children(&self, visitor: &mut impl Visitor) {
+        for attribute in &self.attributes {
+            attribute.accept(AttributeTarget::Struct, visitor);
+        }
+
         for (_name, unresolved_type) in &self.fields {
             unresolved_type.accept(visitor);
         }
@@ -694,6 +721,10 @@ impl NoirTypeAlias {
 
 impl ModuleDeclaration {
     pub fn accept(&self, span: Span, visitor: &mut impl Visitor) {
+        for attribute in &self.outer_attributes {
+            attribute.accept(AttributeTarget::Module, visitor);
+        }
+
         visitor.visit_module_declaration(self, span);
     }
 }
@@ -1295,8 +1326,22 @@ impl Pattern {
 }
 
 impl SecondaryAttribute {
-    pub fn accept(&self, span: Span, visitor: &mut impl Visitor) {
-        visitor.visit_secondary_attribute(self, span);
+    pub fn accept(&self, target: AttributeTarget, visitor: &mut impl Visitor) {
+        if visitor.visit_secondary_attribute(self, target) {
+            self.accept_children(target, visitor);
+        }
+    }
+
+    pub fn accept_children(&self, target: AttributeTarget, visitor: &mut impl Visitor) {
+        if let SecondaryAttribute::Custom(custom) = self {
+            custom.accept(target, visitor);
+        }
+    }
+}
+
+impl CustomAtrribute {
+    pub fn accept(&self, target: AttributeTarget, visitor: &mut impl Visitor) {
+        visitor.visit_custom_attribute(self, target);
     }
 }