Skip to content

Commit

Permalink
feat: don't simplify SSA instructions when creating them from a string (
Browse files Browse the repository at this point in the history
#6948)

Co-authored-by: Tom French <15848336+TomAFrench@users.noreply.github.com>
  • Loading branch information
asterite and TomAFrench authored Jan 6, 2025
1 parent bbdf1a2 commit da94c2b
Show file tree
Hide file tree
Showing 9 changed files with 64 additions and 27 deletions.
26 changes: 20 additions & 6 deletions compiler/noirc_evaluator/src/ssa/function_builder/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,10 @@ pub(crate) struct FunctionBuilder {
finished_functions: Vec<Function>,
call_stack: CallStackId,
error_types: BTreeMap<ErrorSelector, HirType>,

/// Whether instructions are simplified as soon as they are inserted into this builder.
/// This is true by default unless changed to false after constructing a builder.
pub(crate) simplify: bool,
}

impl FunctionBuilder {
Expand All @@ -56,6 +60,7 @@ impl FunctionBuilder {
finished_functions: Vec::new(),
call_stack: CallStackId::root(),
error_types: BTreeMap::default(),
simplify: true,
}
}

Expand Down Expand Up @@ -172,12 +177,21 @@ impl FunctionBuilder {
ctrl_typevars: Option<Vec<Type>>,
) -> InsertInstructionResult {
let block = self.current_block();
self.current_function.dfg.insert_instruction_and_results(
instruction,
block,
ctrl_typevars,
self.call_stack,
)
if self.simplify {
self.current_function.dfg.insert_instruction_and_results(
instruction,
block,
ctrl_typevars,
self.call_stack,
)
} else {
self.current_function.dfg.insert_instruction_and_results_without_simplification(
instruction,
block,
ctrl_typevars,
self.call_stack,
)
}
}

/// Switch to inserting instructions in the given block.
Expand Down
2 changes: 1 addition & 1 deletion compiler/noirc_evaluator/src/ssa/ir/instruction/call.rs
Original file line number Diff line number Diff line change
Expand Up @@ -730,7 +730,7 @@ mod tests {
return v2
}
"#;
let ssa = Ssa::from_str(src).unwrap();
let ssa = Ssa::from_str_simplifying(src).unwrap();

let expected = r#"
brillig(inline) fn main f0 {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -323,7 +323,7 @@ mod test {
v2 = call multi_scalar_mul (v1, v0) -> [Field; 3]
return v2
}"#;
let ssa = Ssa::from_str(src).unwrap();
let ssa = Ssa::from_str_simplifying(src).unwrap();

let expected_src = r#"
acir(inline) fn main f0 {
Expand Down Expand Up @@ -351,7 +351,7 @@ mod test {
return v4
}"#;
let ssa = Ssa::from_str(src).unwrap();
let ssa = Ssa::from_str_simplifying(src).unwrap();
//First point is zero, second scalar is zero, so we should be left with the scalar mul of the last point.
let expected_src = r#"
acir(inline) fn main f0 {
Expand Down Expand Up @@ -379,7 +379,7 @@ mod test {
v4 = call multi_scalar_mul (v3, v2) -> [Field; 3]
return v4
}"#;
let ssa = Ssa::from_str(src).unwrap();
let ssa = Ssa::from_str_simplifying(src).unwrap();
//First and last scalar/point are constant, so we should be left with the msm of the middle point and the folded constant point
let expected_src = r#"
acir(inline) fn main f0 {
Expand Down
2 changes: 1 addition & 1 deletion compiler/noirc_evaluator/src/ssa/opt/constant_folding.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1045,7 +1045,7 @@ mod test {
return
}
";
let ssa = Ssa::from_str(src).unwrap();
let ssa = Ssa::from_str_simplifying(src).unwrap();

let expected = "
acir(inline) fn main f0 {
Expand Down
1 change: 1 addition & 0 deletions compiler/noirc_evaluator/src/ssa/opt/flatten_cfg.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1081,6 +1081,7 @@ mod test {
v23 = mul v20, Field 6
v24 = mul v21, v16
v25 = add v23, v24
enable_side_effects v0
enable_side_effects v3
v26 = cast v3 as Field
v27 = cast v0 as Field
Expand Down
13 changes: 7 additions & 6 deletions compiler/noirc_evaluator/src/ssa/opt/normalize_value_ids.rs
Original file line number Diff line number Diff line change
Expand Up @@ -96,12 +96,13 @@ impl Context {
.requires_ctrl_typevars()
.then(|| vecmap(old_results, |result| old_function.dfg.type_of_value(*result)));

let new_results = new_function.dfg.insert_instruction_and_results(
instruction,
new_block_id,
ctrl_typevars,
new_call_stack,
);
let new_results =
new_function.dfg.insert_instruction_and_results_without_simplification(
instruction,
new_block_id,
ctrl_typevars,
new_call_stack,
);

assert_eq!(old_results.len(), new_results.len());
for (old_result, new_result) in old_results.iter().zip(new_results.results().iter())
Expand Down
11 changes: 6 additions & 5 deletions compiler/noirc_evaluator/src/ssa/parser/into_ssa.rs
Original file line number Diff line number Diff line change
Expand Up @@ -16,8 +16,8 @@ use super::{
};

impl ParsedSsa {
pub(crate) fn into_ssa(self) -> Result<Ssa, SsaError> {
Translator::translate(self)
pub(crate) fn into_ssa(self, simplify: bool) -> Result<Ssa, SsaError> {
Translator::translate(self, simplify)
}
}

Expand All @@ -41,8 +41,8 @@ struct Translator {
}

impl Translator {
fn translate(mut parsed_ssa: ParsedSsa) -> Result<Ssa, SsaError> {
let mut translator = Self::new(&mut parsed_ssa)?;
fn translate(mut parsed_ssa: ParsedSsa, simplify: bool) -> Result<Ssa, SsaError> {
let mut translator = Self::new(&mut parsed_ssa, simplify)?;

// Note that the `new` call above removed the main function,
// so all we are left with are non-main functions.
Expand All @@ -53,12 +53,13 @@ impl Translator {
Ok(translator.finish())
}

fn new(parsed_ssa: &mut ParsedSsa) -> Result<Self, SsaError> {
fn new(parsed_ssa: &mut ParsedSsa, simplify: bool) -> Result<Self, SsaError> {
// A FunctionBuilder must be created with a main Function, so here wer remove it
// from the parsed SSA to avoid adding it twice later on.
let main_function = parsed_ssa.functions.remove(0);
let main_id = FunctionId::test_new(0);
let mut builder = FunctionBuilder::new(main_function.external_name.clone(), main_id);
builder.simplify = simplify;
builder.set_runtime(main_function.runtime_type);

// Map function names to their IDs so calls can be resolved
Expand Down
18 changes: 13 additions & 5 deletions compiler/noirc_evaluator/src/ssa/parser/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -32,16 +32,24 @@ mod token;

impl Ssa {
/// Creates an Ssa object from the given string.
///
/// Note that the resulting Ssa might not be exactly the same as the given string.
/// This is because, internally, the Ssa is built using a `FunctionBuilder`, so
/// some instructions might be simplified while they are inserted.
pub(crate) fn from_str(src: &str) -> Result<Ssa, SsaErrorWithSource> {
Self::from_str_impl(src, false)
}

/// Creates an Ssa object from the given string but trying to simplify
/// each parsed instruction as it's inserted into the final SSA.
pub(crate) fn from_str_simplifying(src: &str) -> Result<Ssa, SsaErrorWithSource> {
Self::from_str_impl(src, true)
}

fn from_str_impl(src: &str, simplify: bool) -> Result<Ssa, SsaErrorWithSource> {
let mut parser =
Parser::new(src).map_err(|err| SsaErrorWithSource::parse_error(err, src))?;
let parsed_ssa =
parser.parse_ssa().map_err(|err| SsaErrorWithSource::parse_error(err, src))?;
parsed_ssa.into_ssa().map_err(|error| SsaErrorWithSource { src: src.to_string(), error })
parsed_ssa
.into_ssa(simplify)
.map_err(|error| SsaErrorWithSource { src: src.to_string(), error })
}
}

Expand Down
12 changes: 12 additions & 0 deletions compiler/noirc_evaluator/src/ssa/parser/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -502,3 +502,15 @@ fn test_function_type() {
";
assert_ssa_roundtrip(src);
}

#[test]
fn test_does_not_simplify() {
let src = "
acir(inline) fn main f0 {
b0():
v2 = add Field 1, Field 2
return v2
}
";
assert_ssa_roundtrip(src);
}

0 comments on commit da94c2b

Please sign in to comment.