Skip to content

Commit 89e4859

Browse files
author
AztecBot
committed
feat(ssa): Hoist MakeArray instructions during loop invariant code motion (noir-lang/noir#6782)
feat: add `(x | 1)` optimization for booleans (noir-lang/noir#6795) feat: `nargo test -q` (or `nargo test --format terse`) (noir-lang/noir#6776) fix: disable failure persistance in nargo test fuzzing (noir-lang/noir#6777) feat(cli): Verify `return` against ABI and `Prover.toml` (noir-lang/noir#6765) chore(ssa): Activate loop invariant code motion on ACIR functions (noir-lang/noir#6785) fix: use extension in docs link so it also works on GitHub (noir-lang/noir#6787) fix: optimizer to keep track of changing opcode locations (noir-lang/noir#6781) fix: Minimal change to avoid reverting entire PR #6685 (noir-lang/noir#6778)
2 parents 08453a9 + 3b86c79 commit 89e4859

File tree

24 files changed

+684
-193
lines changed

24 files changed

+684
-193
lines changed

.noir-sync-commit

+1-1
Original file line numberDiff line numberDiff line change
@@ -1 +1 @@
1-
0925a332dbaa561aad195c143079588158498dad
1+
b88db67a4fa92f861329105fb732a7b1309620fe

noir/noir-repo/.github/workflows/test-js-packages.yml

+11-11
Original file line numberDiff line numberDiff line change
@@ -534,7 +534,7 @@ jobs:
534534
- name: Build list of libraries
535535
id: get_critical_libraries
536536
run: |
537-
LIBRARIES=$(grep -Po "^https://github.com/\K.+" ./CRITICAL_NOIR_LIBRARIES | jq -R -s -c 'split("\n") | map(select(. != "")) | map({ repo: ., path: "./"})')
537+
LIBRARIES=$(grep -Po "^https://github.com/\K.+" ./CRITICAL_NOIR_LIBRARIES | jq -R -s -c 'split("\n") | map(select(. != "")) | map({ repo: ., path: ""})')
538538
echo "libraries=$LIBRARIES"
539539
echo "libraries=$LIBRARIES" >> $GITHUB_OUTPUT
540540
env:
@@ -551,15 +551,15 @@ jobs:
551551
matrix:
552552
project: ${{ fromJson( needs.critical-library-list.outputs.libraries )}}
553553
include:
554-
- project: { repo: AztecProtocol/aztec-packages, path: ./noir-projects/aztec-nr }
555-
- project: { repo: AztecProtocol/aztec-packages, path: ./noir-projects/noir-contracts }
556-
- project: { repo: AztecProtocol/aztec-packages, path: ./noir-projects/noir-protocol-circuits/crates/parity-lib }
557-
- project: { repo: AztecProtocol/aztec-packages, path: ./noir-projects/noir-protocol-circuits/crates/private-kernel-lib }
558-
- project: { repo: AztecProtocol/aztec-packages, path: ./noir-projects/noir-protocol-circuits/crates/reset-kernel-lib }
559-
- project: { repo: AztecProtocol/aztec-packages, path: ./noir-projects/noir-protocol-circuits/crates/rollup-lib }
560-
- project: { repo: AztecProtocol/aztec-packages, path: ./noir-projects/noir-protocol-circuits/crates/types }
561-
562-
name: Check external repo - ${{ matrix.project.repo }}
554+
- project: { repo: AztecProtocol/aztec-packages, path: noir-projects/aztec-nr }
555+
- project: { repo: AztecProtocol/aztec-packages, path: noir-projects/noir-contracts }
556+
- project: { repo: AztecProtocol/aztec-packages, path: noir-projects/noir-protocol-circuits/crates/parity-lib }
557+
- project: { repo: AztecProtocol/aztec-packages, path: noir-projects/noir-protocol-circuits/crates/private-kernel-lib }
558+
- project: { repo: AztecProtocol/aztec-packages, path: noir-projects/noir-protocol-circuits/crates/reset-kernel-lib }
559+
- project: { repo: AztecProtocol/aztec-packages, path: noir-projects/noir-protocol-circuits/crates/rollup-lib }
560+
- project: { repo: AztecProtocol/aztec-packages, path: noir-projects/noir-protocol-circuits/crates/types }
561+
562+
name: Check external repo - ${{ matrix.project.repo }}/${{ matrix.project.path }}
563563
steps:
564564
- name: Checkout
565565
uses: actions/checkout@v4
@@ -591,7 +591,7 @@ jobs:
591591
592592
- name: Run nargo test
593593
working-directory: ./test-repo/${{ matrix.project.path }}
594-
run: nargo test --silence-warnings
594+
run: nargo test -q --silence-warnings
595595
env:
596596
NARGO_IGNORE_TEST_FAILURES_FROM_FOREIGN_CALLS: true
597597

noir/noir-repo/acvm-repo/acvm/src/compiler/mod.rs

+5-4
Original file line numberDiff line numberDiff line change
@@ -82,15 +82,16 @@ pub fn compile<F: AcirField>(
8282
acir: Circuit<F>,
8383
expression_width: ExpressionWidth,
8484
) -> (Circuit<F>, AcirTransformationMap) {
85-
let initial_opcode_positions = (0..acir.opcodes.len()).collect::<Vec<_>>();
85+
let acir_opcode_positions = (0..acir.opcodes.len()).collect::<Vec<_>>();
86+
8687
if MAX_OPTIMIZER_PASSES == 0 {
87-
let transformation_map = AcirTransformationMap::new(&initial_opcode_positions);
88-
return (acir, transformation_map);
88+
return (acir, AcirTransformationMap::new(&acir_opcode_positions));
8989
}
90+
9091
let mut pass = 0;
9192
let mut prev_opcodes_hash = fxhash::hash64(&acir.opcodes);
9293
let mut prev_acir = acir;
93-
let mut prev_acir_opcode_positions = initial_opcode_positions;
94+
let mut prev_acir_opcode_positions = acir_opcode_positions;
9495

9596
// For most test programs it would be enough to only loop `transform_internal`,
9697
// but some of them don't stabilize unless we also repeat the backend agnostic optimizations.

noir/noir-repo/acvm-repo/acvm/src/compiler/optimizers/mod.rs

+1
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,7 @@ pub fn optimize<F: AcirField>(acir: Circuit<F>) -> (Circuit<F>, AcirTransformati
2424
// Track original acir opcode positions throughout the transformation passes of the compilation
2525
// by applying the modifications done to the circuit opcodes and also to the opcode_positions (delete and insert)
2626
let acir_opcode_positions = (0..acir.opcodes.len()).collect();
27+
2728
let (mut acir, new_opcode_positions) = optimize_internal(acir, acir_opcode_positions);
2829

2930
let transformation_map = AcirTransformationMap::new(&new_opcode_positions);

noir/noir-repo/compiler/noirc_evaluator/src/ssa/ir/instruction/binary.rs

+4
Original file line numberDiff line numberDiff line change
@@ -256,6 +256,10 @@ impl Binary {
256256
if rhs_is_zero {
257257
return SimplifyResult::SimplifiedTo(self.lhs);
258258
}
259+
if operand_type == NumericType::bool() && (lhs_is_one || rhs_is_one) {
260+
let one = dfg.make_constant(FieldElement::one(), operand_type);
261+
return SimplifyResult::SimplifiedTo(one);
262+
}
259263
if dfg.resolve(self.lhs) == dfg.resolve(self.rhs) {
260264
return SimplifyResult::SimplifiedTo(self.lhs);
261265
}

noir/noir-repo/compiler/noirc_evaluator/src/ssa/ir/instruction/call.rs

+8-11
Original file line numberDiff line numberDiff line change
@@ -480,20 +480,14 @@ fn simplify_slice_push_back(
480480
}
481481

482482
fn simplify_slice_pop_back(
483-
element_type: Type,
483+
slice_type: Type,
484484
arguments: &[ValueId],
485485
dfg: &mut DataFlowGraph,
486486
block: BasicBlockId,
487487
call_stack: CallStack,
488488
) -> SimplifyResult {
489-
let element_types = match element_type.clone() {
490-
Type::Slice(element_types) | Type::Array(element_types, _) => element_types,
491-
_ => {
492-
unreachable!("ICE: Expected slice or array, but got {element_type}");
493-
}
494-
};
495-
496-
let element_count = element_type.element_size();
489+
let element_types = slice_type.element_types();
490+
let element_count = element_types.len();
497491
let mut results = VecDeque::with_capacity(element_count + 1);
498492

499493
let new_slice_length = update_slice_length(arguments[0], dfg, BinaryOp::Sub, block);
@@ -507,14 +501,17 @@ fn simplify_slice_pop_back(
507501
flattened_len = update_slice_length(flattened_len, dfg, BinaryOp::Sub, block);
508502

509503
// We must pop multiple elements in the case of a slice of tuples
510-
for _ in 0..element_count {
504+
// Iterating through element types in reverse here since we're popping from the end
505+
for element_type in element_types.iter().rev() {
511506
let get_last_elem_instr =
512507
Instruction::ArrayGet { array: arguments[1], index: flattened_len };
508+
509+
let element_type = Some(vec![element_type.clone()]);
513510
let get_last_elem = dfg
514511
.insert_instruction_and_results(
515512
get_last_elem_instr,
516513
block,
517-
Some(element_types.to_vec()),
514+
element_type,
518515
call_stack.clone(),
519516
)
520517
.first();

noir/noir-repo/compiler/noirc_evaluator/src/ssa/ir/instruction/call/blackbox.rs

+4
Original file line numberDiff line numberDiff line change
@@ -173,7 +173,11 @@ pub(super) fn simplify_msm(
173173
var_scalars.push(zero);
174174
let result_x = dfg.make_constant(result_x, NumericType::NativeField);
175175
let result_y = dfg.make_constant(result_y, NumericType::NativeField);
176+
177+
// Pushing a bool here is intentional, multi_scalar_mul takes two arguments:
178+
// `points: [(Field, Field, bool); N]` and `scalars: [(Field, Field); N]`.
176179
let result_is_infinity = dfg.make_constant(result_is_infinity, NumericType::bool());
180+
177181
var_points.push(result_x);
178182
var_points.push(result_y);
179183
var_points.push(result_is_infinity);

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

+110-7
Original file line numberDiff line numberDiff line change
@@ -13,7 +13,7 @@ use fxhash::{FxHashMap as HashMap, FxHashSet as HashSet};
1313
use crate::ssa::{
1414
ir::{
1515
basic_block::BasicBlockId,
16-
function::{Function, RuntimeType},
16+
function::Function,
1717
function_inserter::FunctionInserter,
1818
instruction::{Instruction, InstructionId},
1919
types::Type,
@@ -27,12 +27,7 @@ use super::unrolling::{Loop, Loops};
2727
impl Ssa {
2828
#[tracing::instrument(level = "trace", skip(self))]
2929
pub(crate) fn loop_invariant_code_motion(mut self) -> Ssa {
30-
let brillig_functions = self
31-
.functions
32-
.iter_mut()
33-
.filter(|(_, func)| matches!(func.runtime(), RuntimeType::Brillig(_)));
34-
35-
for (_, function) in brillig_functions {
30+
for function in self.functions.values_mut() {
3631
function.loop_invariant_code_motion();
3732
}
3833

@@ -63,6 +58,7 @@ impl Loops {
6358
}
6459

6560
context.map_dependent_instructions();
61+
context.inserter.map_data_bus_in_place();
6662
}
6763
}
6864

@@ -113,6 +109,22 @@ impl<'f> LoopInvariantContext<'f> {
113109

114110
if hoist_invariant {
115111
self.inserter.push_instruction(instruction_id, pre_header);
112+
113+
// If we are hoisting a MakeArray instruction,
114+
// we need to issue an extra inc_rc in case they are mutated afterward.
115+
if matches!(
116+
self.inserter.function.dfg[instruction_id],
117+
Instruction::MakeArray { .. }
118+
) {
119+
let result =
120+
self.inserter.function.dfg.instruction_results(instruction_id)[0];
121+
let inc_rc = Instruction::IncrementRc { value: result };
122+
let call_stack = self.inserter.function.dfg.get_call_stack(instruction_id);
123+
self.inserter
124+
.function
125+
.dfg
126+
.insert_instruction_and_results(inc_rc, *block, None, call_stack);
127+
}
116128
} else {
117129
self.inserter.push_instruction(instruction_id, *block);
118130
}
@@ -190,6 +202,7 @@ impl<'f> LoopInvariantContext<'f> {
190202
});
191203

192204
let can_be_deduplicated = instruction.can_be_deduplicated(self.inserter.function, false)
205+
|| matches!(instruction, Instruction::MakeArray { .. })
193206
|| self.can_be_deduplicated_from_upper_bound(&instruction);
194207

195208
is_loop_invariant && can_be_deduplicated
@@ -559,4 +572,94 @@ mod test {
559572
let ssa = ssa.loop_invariant_code_motion();
560573
assert_normalized_ssa_equals(ssa, expected);
561574
}
575+
576+
#[test]
577+
fn insert_inc_rc_when_moving_make_array() {
578+
// SSA for the following program:
579+
//
580+
// unconstrained fn main(x: u32, y: u32) {
581+
// let mut a1 = [1, 2, 3, 4, 5];
582+
// a1[x] = 64;
583+
// for i in 0 .. 5 {
584+
// let mut a2 = [1, 2, 3, 4, 5];
585+
// a2[y + i] = 128;
586+
// foo(a2);
587+
// }
588+
// foo(a1);
589+
// }
590+
//
591+
// We want to make sure move a loop invariant make_array instruction,
592+
// to account for whether that array has been marked as mutable.
593+
// To do so, we increment the reference counter on the array we are moving.
594+
// In the SSA below, we want to move `v42` out of the loop.
595+
let src = "
596+
brillig(inline) fn main f0 {
597+
b0(v0: u32, v1: u32):
598+
v8 = make_array [Field 1, Field 2, Field 3, Field 4, Field 5] : [Field; 5]
599+
v9 = allocate -> &mut [Field; 5]
600+
v11 = array_set v8, index v0, value Field 64
601+
v13 = add v0, u32 1
602+
store v11 at v9
603+
jmp b1(u32 0)
604+
b1(v2: u32):
605+
v16 = lt v2, u32 5
606+
jmpif v16 then: b3, else: b2
607+
b2():
608+
v17 = load v9 -> [Field; 5]
609+
call f1(v17)
610+
return
611+
b3():
612+
v19 = make_array [Field 1, Field 2, Field 3, Field 4, Field 5] : [Field; 5]
613+
v20 = allocate -> &mut [Field; 5]
614+
v21 = add v1, v2
615+
v23 = array_set v19, index v21, value Field 128
616+
call f1(v23)
617+
v25 = add v2, u32 1
618+
jmp b1(v25)
619+
}
620+
brillig(inline) fn foo f1 {
621+
b0(v0: [Field; 5]):
622+
return
623+
}
624+
";
625+
626+
let ssa = Ssa::from_str(src).unwrap();
627+
628+
// We expect the `make_array` at the top of `b3` to be replaced with an `inc_rc`
629+
// of the newly hoisted `make_array` at the end of `b0`.
630+
let expected = "
631+
brillig(inline) fn main f0 {
632+
b0(v0: u32, v1: u32):
633+
v8 = make_array [Field 1, Field 2, Field 3, Field 4, Field 5] : [Field; 5]
634+
v9 = allocate -> &mut [Field; 5]
635+
v11 = array_set v8, index v0, value Field 64
636+
v13 = add v0, u32 1
637+
store v11 at v9
638+
v14 = make_array [Field 1, Field 2, Field 3, Field 4, Field 5] : [Field; 5]
639+
jmp b1(u32 0)
640+
b1(v2: u32):
641+
v17 = lt v2, u32 5
642+
jmpif v17 then: b3, else: b2
643+
b2():
644+
v18 = load v9 -> [Field; 5]
645+
call f1(v18)
646+
return
647+
b3():
648+
inc_rc v14
649+
v20 = allocate -> &mut [Field; 5]
650+
v21 = add v1, v2
651+
v23 = array_set v14, index v21, value Field 128
652+
call f1(v23)
653+
v25 = add v2, u32 1
654+
jmp b1(v25)
655+
}
656+
brillig(inline) fn foo f1 {
657+
b0(v0: [Field; 5]):
658+
return
659+
}
660+
";
661+
662+
let ssa = ssa.loop_invariant_code_motion();
663+
assert_normalized_ssa_equals(ssa, expected);
664+
}
562665
}

noir/noir-repo/docs/docs/getting_started/noir_installation.md

+1-1
Original file line numberDiff line numberDiff line change
@@ -93,7 +93,7 @@ step 2: Follow the [Noirup instructions](#installing-noirup).
9393

9494
## Setting up shell completions
9595

96-
Once `nargo` is installed, you can [set up shell completions for it](setting_up_shell_completions).
96+
Once `nargo` is installed, you can [set up shell completions for it](setting_up_shell_completions.md).
9797

9898
## Uninstalling Nargo
9999

noir/noir-repo/scripts/check-critical-libraries.sh

+1-1
Original file line numberDiff line numberDiff line change
@@ -31,7 +31,7 @@ for REPO in ${REPOS_TO_CHECK[@]}; do
3131
TAG=$(getLatestReleaseTagForRepo $REPO)
3232
git clone $REPO -c advice.detachedHead=false --depth 1 --branch $TAG $TMP_DIR
3333

34-
nargo test --program-dir $TMP_DIR
34+
nargo test -q --program-dir $TMP_DIR
3535

3636
rm -rf $TMP_DIR
3737
done
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,3 @@
11
x = 1
22
y = 1
3-
return = 5
3+
return = 7
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,7 @@
1+
[package]
2+
name = "return_twice"
3+
version = "0.1.0"
4+
type = "bin"
5+
authors = [""]
6+
7+
[dependencies]
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,2 @@
1+
return = [100, 100]
2+
in0 = 10
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
pub fn main(in0: Field) -> pub (Field, Field) {
2+
let out0 = (in0 * in0);
3+
let out1 = (in0 * in0);
4+
(out0, out1)
5+
}

noir/noir-repo/test_programs/gates_report_brillig_execution.sh

-2
Original file line numberDiff line numberDiff line change
@@ -8,8 +8,6 @@ excluded_dirs=(
88
"double_verify_nested_proof"
99
"overlapping_dep_and_mod"
1010
"comptime_println"
11-
# Takes a very long time to execute as large loops do not get simplified.
12-
"regression_4709"
1311
# bit sizes for bigint operation doesn't match up.
1412
"bigint"
1513
# Expected to fail as test asserts on which runtime it is in.
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,7 @@
1+
[package]
2+
name = "assert_message"
3+
type = "bin"
4+
authors = [""]
5+
compiler_version = ">=0.23.0"
6+
7+
[dependencies]
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,15 @@
1+
// Have to use inputs otherwise the ACIR gets optimized away due to constants.
2+
// With the original ACIR the optimizations can rearrange or merge opcodes,
3+
// which might end up getting out of sync with assertion messages.
4+
#[test(should_fail_with = "first")]
5+
fn test_assert_message_preserved_during_optimization(a: Field, b: Field, c: Field) {
6+
if (a + b) * (a - b) != c {
7+
assert((a + b) * (a - b) == c, "first");
8+
assert((a - b) * (a + b) == c, "second");
9+
assert((a + b) * (a - b) == c, "third");
10+
assert((2 * a + b) * (a - b / 2) == c * c, "fourth");
11+
} else {
12+
// The fuzzer might have generated a passing test.
13+
assert((a + b) * (a - b) != c, "first");
14+
}
15+
}
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,9 @@
1-
21
#[test(should_fail_with = "42 is not allowed")]
32
fn finds_magic_value(x: u32) {
43
let x = x as u64;
5-
assert(2 * x != 42, "42 is not allowed");
4+
if x == 21 {
5+
assert(2 * x != 42, "42 is not allowed");
6+
} else {
7+
assert(2 * x == 42, "42 is not allowed");
8+
}
69
}

0 commit comments

Comments
 (0)