Skip to content

Commit

Permalink
chore(ci): Add non determinism check and fixes (#6847)
Browse files Browse the repository at this point in the history
  • Loading branch information
vezenovm authored Dec 18, 2024
1 parent 9797451 commit 16c0a82
Show file tree
Hide file tree
Showing 6 changed files with 33 additions and 11 deletions.
4 changes: 4 additions & 0 deletions compiler/noirc_driver/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -150,6 +150,10 @@ pub struct CompileOptions {
/// A lower value keeps the original program if it was smaller, even if it has more jumps.
#[arg(long, hide = true, allow_hyphen_values = true)]
pub max_bytecode_increase_percent: Option<i32>,

/// Used internally to test for non-determinism in the compiler.
#[clap(long, hide = true)]
pub check_non_determinism: bool,
}

pub fn parse_expression_width(input: &str) -> Result<ExpressionWidth, std::io::Error> {
Expand Down
4 changes: 2 additions & 2 deletions compiler/noirc_evaluator/src/brillig/brillig_ir/artifact.rs
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ pub(crate) struct GeneratedBrillig<F> {
pub(crate) locations: BTreeMap<OpcodeLocation, CallStack>,
pub(crate) error_types: BTreeMap<ErrorSelector, ErrorType>,
pub(crate) name: String,
pub(crate) procedure_locations: HashMap<ProcedureId, (OpcodeLocation, OpcodeLocation)>,
pub(crate) procedure_locations: BTreeMap<ProcedureId, (OpcodeLocation, OpcodeLocation)>,
}

#[derive(Default, Debug, Clone)]
Expand Down Expand Up @@ -61,7 +61,7 @@ pub(crate) struct BrilligArtifact<F> {
/// This is created as artifacts are linked together and allows us to determine
/// which opcodes originate from reusable procedures.s
/// The range is inclusive for both start and end opcode locations.
pub(crate) procedure_locations: HashMap<ProcedureId, (OpcodeLocation, OpcodeLocation)>,
pub(crate) procedure_locations: BTreeMap<ProcedureId, (OpcodeLocation, OpcodeLocation)>,
}

/// A pointer to a location in the opcode.
Expand Down
6 changes: 4 additions & 2 deletions compiler/noirc_evaluator/src/ssa/opt/unrolling.rs
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,8 @@
//!
//! When unrolling ACIR code, we remove reference count instructions because they are
//! only used by Brillig bytecode.
use std::collections::BTreeSet;

use acvm::{acir::AcirField, FieldElement};
use im::HashSet;

Expand Down Expand Up @@ -117,7 +119,7 @@ pub(super) struct Loop {
back_edge_start: BasicBlockId,

/// All the blocks contained within the loop, including `header` and `back_edge_start`.
pub(super) blocks: HashSet<BasicBlockId>,
pub(super) blocks: BTreeSet<BasicBlockId>,
}

pub(super) struct Loops {
Expand Down Expand Up @@ -238,7 +240,7 @@ impl Loop {
back_edge_start: BasicBlockId,
cfg: &ControlFlowGraph,
) -> Self {
let mut blocks = HashSet::default();
let mut blocks = BTreeSet::default();
blocks.insert(header);

let mut insert = |block, stack: &mut Vec<BasicBlockId>| {
Expand Down
13 changes: 6 additions & 7 deletions compiler/noirc_frontend/src/ast/statement.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
use std::fmt::Display;
use std::sync::atomic::{AtomicU32, Ordering};

use acvm::acir::AcirField;
use acvm::FieldElement;
Expand Down Expand Up @@ -841,10 +840,9 @@ impl ForRange {
block: Expression,
for_loop_span: Span,
) -> Statement {
/// Counter used to generate unique names when desugaring
/// code in the parser requires the creation of fresh variables.
/// The parser is stateless so this is a static global instead.
static UNIQUE_NAME_COUNTER: AtomicU32 = AtomicU32::new(0);
// Counter used to generate unique names when desugaring
// code in the parser requires the creation of fresh variables.
let mut unique_name_counter: u32 = 0;

match self {
ForRange::Range(..) => {
Expand All @@ -855,7 +853,8 @@ impl ForRange {
let start_range = ExpressionKind::integer(FieldElement::zero());
let start_range = Expression::new(start_range, array_span);

let next_unique_id = UNIQUE_NAME_COUNTER.fetch_add(1, Ordering::Relaxed);
let next_unique_id = unique_name_counter;
unique_name_counter += 1;
let array_name = format!("$i{next_unique_id}");
let array_span = array.span;
let array_ident = Ident::new(array_name, array_span);
Expand Down Expand Up @@ -888,7 +887,7 @@ impl ForRange {
}));
let end_range = Expression::new(end_range, array_span);

let next_unique_id = UNIQUE_NAME_COUNTER.fetch_add(1, Ordering::Relaxed);
let next_unique_id = unique_name_counter;
let index_name = format!("$i{next_unique_id}");
let fresh_identifier = Ident::new(index_name.clone(), array_span);

Expand Down
3 changes: 3 additions & 0 deletions tooling/nargo_cli/build.rs
Original file line number Diff line number Diff line change
Expand Up @@ -215,6 +215,9 @@ fn test_{test_name}(force_brillig: ForceBrillig, inliner_aggressiveness: Inliner
// Set the maximum increase so that part of the optimization is exercised (it might fail).
nargo.arg("--max-bytecode-increase-percent");
nargo.arg("50");
// Check whether the test case is non-deterministic
nargo.arg("--check-non-determinism");
}}
{test_content}
Expand Down
14 changes: 14 additions & 0 deletions tooling/nargo_cli/src/cli/compile_cmd.rs
Original file line number Diff line number Diff line change
Expand Up @@ -216,6 +216,20 @@ fn compile_programs(
cached_program,
)?;

if compile_options.check_non_determinism {
let (program_two, _) = compile_program(
file_manager,
parsed_files,
workspace,
package,
compile_options,
load_cached_program(package),
)?;
if fxhash::hash64(&program) != fxhash::hash64(&program_two) {
panic!("Non deterministic result compiling {}", package.name);
}
}

// Choose the target width for the final, backend specific transformation.
let target_width =
get_target_width(package.expression_width, compile_options.expression_width);
Expand Down

0 comments on commit 16c0a82

Please sign in to comment.