Skip to content

Commit

Permalink
chore: perform dead instruction elimination through std::as_witness (
Browse files Browse the repository at this point in the history
…#5123)

# Description

## Problem\*

Resolves <!-- Link to GitHub Issue -->

## Summary\*

This PR adds handling for `std::as_witness` in dead instruction
elimination. i.e. if we're asking for an SSA value to be assigned to a
witness when that value would not otherwise be represented in the ACIR
then we can remove it.

This issue can be seen in #5122 where making two calls to the same
function with the same inputs and asserting that the outputs are the
same results in constraints whereas this can be determined at runtime.

## Additional Context



## Documentation\*

Check one:
- [x] No documentation needed.
- [ ] Documentation included in this PR.
- [ ] **[For Experimental Features]** Documentation to be submitted in a
separate PR.

# PR Checklist\*

- [x] I have tested the changes locally.
- [x] I have formatted the changes with [Prettier](https://prettier.io/)
and/or `cargo fmt` on default settings.
  • Loading branch information
TomAFrench committed May 28, 2024
1 parent e4eb5f5 commit b359bac
Show file tree
Hide file tree
Showing 2 changed files with 49 additions and 2 deletions.
2 changes: 1 addition & 1 deletion compiler/noirc_evaluator/src/ssa/ir/dfg.rs
Original file line number Diff line number Diff line change
Expand Up @@ -284,7 +284,7 @@ impl DataFlowGraph {
intrinsic_value_id
}

pub(crate) fn get_intrinsic(&mut self, intrinsic: Intrinsic) -> Option<&ValueId> {
pub(crate) fn get_intrinsic(&self, intrinsic: Intrinsic) -> Option<&ValueId> {
self.intrinsics.get(&intrinsic)
}

Expand Down
49 changes: 48 additions & 1 deletion compiler/noirc_evaluator/src/ssa/opt/die.rs
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ use crate::ssa::{
basic_block::{BasicBlock, BasicBlockId},
dfg::DataFlowGraph,
function::Function,
instruction::{Instruction, InstructionId},
instruction::{Instruction, InstructionId, Intrinsic},
post_order::PostOrder,
value::{Value, ValueId},
},
Expand Down Expand Up @@ -111,6 +111,10 @@ impl Context {
if instruction.can_eliminate_if_unused(&function.dfg) {
let results = function.dfg.instruction_results(instruction_id);
results.iter().all(|result| !self.used_values.contains(result))
} else if let Instruction::Call { func, arguments } = instruction {
// TODO: make this more general for instructions which don't have results but have side effects "sometimes" like `Intrinsic::AsWitness`
let as_witness_id = function.dfg.get_intrinsic(Intrinsic::AsWitness);
as_witness_id == Some(func) && !self.used_values.contains(&arguments[0])
} else {
// If the instruction has side effects we should never remove it.
false
Expand Down Expand Up @@ -256,4 +260,47 @@ mod test {
assert_eq!(main.dfg[main.entry_block()].instructions().len(), 1);
assert_eq!(main.dfg[b1].instructions().len(), 6);
}

#[test]
fn as_witness_die() {
// fn main f0 {
// b0(v0: Field):
// v1 = add v0, Field 1
// v2 = add v0, Field 2
// call as_witness(v2)
// return v1
// }
let main_id = Id::test_new(0);

// Compiling main
let mut builder = FunctionBuilder::new("main".into(), main_id);
let v0 = builder.add_parameter(Type::field());

let one = builder.field_constant(1u128);
let two = builder.field_constant(2u128);

let v1 = builder.insert_binary(v0, BinaryOp::Add, one);
let v2 = builder.insert_binary(v0, BinaryOp::Add, two);
let as_witness = builder.import_intrinsic("as_witness").unwrap();
builder.insert_call(as_witness, vec![v2], Vec::new());
builder.terminate_with_return(vec![v1]);

let ssa = builder.finish();
let main = ssa.main();

// The instruction count never includes the terminator instruction
assert_eq!(main.dfg[main.entry_block()].instructions().len(), 3);

// Expected output:
//
// acir(inline) fn main f0 {
// b0(v0: Field):
// v3 = add v0, Field 1
// return v3
// }
let ssa = ssa.dead_instruction_elimination();
let main = ssa.main();

assert_eq!(main.dfg[main.entry_block()].instructions().len(), 1);
}
}

0 comments on commit b359bac

Please sign in to comment.