Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Sync from noir #8378

Merged
merged 17 commits into from
Sep 5, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .noir-sync-commit
Original file line number Diff line number Diff line change
@@ -1 +1 @@
44cf9a2140bc06b550d4b46966f1637598ac11a7
b84009ca428a5790acf53a6c027146b706170574
4 changes: 2 additions & 2 deletions noir/noir-repo/aztec_macros/src/utils/ast_utils.rs
Original file line number Diff line number Diff line change
Expand Up @@ -187,8 +187,8 @@ pub fn check_trait_method_implemented(trait_impl: &NoirTraitImpl, method_name: &

/// Checks if an attribute is a custom attribute with a specific name
pub fn is_custom_attribute(attr: &SecondaryAttribute, attribute_name: &str) -> bool {
if let SecondaryAttribute::Custom(custom_attr) = attr {
custom_attr.as_str() == attribute_name
if let SecondaryAttribute::Custom(custom_attribute) = attr {
custom_attribute.contents.as_str() == attribute_name
} else {
false
}
Expand Down
1 change: 1 addition & 0 deletions noir/noir-repo/aztec_macros/src/utils/parse_utils.rs
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,7 @@ fn empty_item(item: &mut Item) {
ItemKind::Import(use_tree, _) => empty_use_tree(use_tree),
ItemKind::Struct(noir_struct) => empty_noir_struct(noir_struct),
ItemKind::TypeAlias(noir_type_alias) => empty_noir_type_alias(noir_type_alias),
ItemKind::InnerAttribute(_) => (),
}
}

Expand Down
10 changes: 7 additions & 3 deletions noir/noir-repo/compiler/noirc_driver/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -452,9 +452,13 @@ fn compile_contract_inner(
.attributes
.secondary
.iter()
.filter_map(
|attr| if let SecondaryAttribute::Custom(tag) = attr { Some(tag) } else { None },
)
.filter_map(|attr| {
if let SecondaryAttribute::Custom(attribute) = attr {
Some(&attribute.contents)
} else {
None
}
})
.cloned()
.collect();

Expand Down
4 changes: 4 additions & 0 deletions noir/noir-repo/compiler/noirc_errors/src/position.rs
Original file line number Diff line number Diff line change
Expand Up @@ -103,6 +103,10 @@ impl Span {
let other_distance = other.end() - other.start();
self_distance < other_distance
}

pub fn shift_by(&self, offset: u32) -> Span {
Self::from(self.start() + offset..self.end() + offset)
}
}

impl From<Span> for Range<usize> {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -648,7 +648,11 @@ impl<'block> BrilligBlock<'block> {
};

let index_variable = self.convert_ssa_single_addr_value(*index, dfg);
self.validate_array_index(array_variable, index_variable);

if !dfg.is_safe_index(*index, *array) {
self.validate_array_index(array_variable, index_variable);
}

self.retrieve_variable_from_array(
array_pointer,
index_variable,
Expand All @@ -667,7 +671,11 @@ impl<'block> BrilligBlock<'block> {
result_ids[0],
dfg,
);
self.validate_array_index(source_variable, index_register);

if !dfg.is_safe_index(*index, *array) {
self.validate_array_index(source_variable, index_register);
}

self.convert_ssa_array_set(
source_variable,
destination_variable,
Expand Down
3 changes: 3 additions & 0 deletions noir/noir-repo/compiler/noirc_evaluator/src/errors.rs
Original file line number Diff line number Diff line change
Expand Up @@ -88,6 +88,7 @@ impl From<SsaReport> for FileDiagnostic {
InternalBug::IndependentSubgraph { call_stack } => {
("There is no path from the output of this brillig call to either return values or inputs of the circuit, which creates an independent subgraph. This is quite likely a soundness vulnerability".to_string(),call_stack)
}
InternalBug::AssertFailed { call_stack } => ("As a result, the compiled circuit is ensured to fail. Other assertions may also fail during execution".to_string(), call_stack)
};
let call_stack = vecmap(call_stack, |location| location);
let file_id = call_stack.last().map(|location| location.file).unwrap_or_default();
Expand All @@ -111,6 +112,8 @@ pub enum InternalWarning {
pub enum InternalBug {
#[error("Input to brillig function is in a separate subgraph to output")]
IndependentSubgraph { call_stack: CallStack },
#[error("Assertion is always false")]
AssertFailed { call_stack: CallStack },
}

#[derive(Debug, PartialEq, Eq, Clone, Error)]
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ use super::big_int::BigIntContext;
use super::generated_acir::{BrilligStdlibFunc, GeneratedAcir, PLACEHOLDER_BRILLIG_INDEX};
use crate::brillig::brillig_gen::brillig_directive;
use crate::brillig::brillig_ir::artifact::GeneratedBrillig;
use crate::errors::{InternalError, RuntimeError, SsaReport};
use crate::errors::{InternalBug, InternalError, RuntimeError, SsaReport};
use crate::ssa::acir_gen::{AcirDynamicArray, AcirValue};
use crate::ssa::ir::dfg::CallStack;
use crate::ssa::ir::types::Type as SsaType;
Expand Down Expand Up @@ -126,6 +126,8 @@ pub(crate) struct AcirContext<F: AcirField> {
big_int_ctx: BigIntContext,

expression_width: ExpressionWidth,

pub(crate) warnings: Vec<SsaReport>,
}

impl<F: AcirField> AcirContext<F> {
Expand Down Expand Up @@ -518,6 +520,12 @@ impl<F: AcirField> AcirContext<F> {
self.mark_variables_equivalent(lhs, rhs)?;
return Ok(());
}
if diff_expr.is_const() {
// Constraint is always false
self.warnings.push(SsaReport::Bug(InternalBug::AssertFailed {
call_stack: self.get_call_stack(),
}));
}

self.acir_ir.assert_is_zero(diff_expr);
if let Some(payload) = assert_message {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -453,6 +453,7 @@ impl<'a> Context<'a> {
}

warnings.extend(return_warnings);
warnings.extend(self.acir_context.warnings.clone());

// Add the warnings from the alter Ssa passes
Ok(self.acir_context.finish(input_witness, return_witnesses, warnings))
Expand Down
5 changes: 2 additions & 3 deletions noir/noir-repo/compiler/noirc_evaluator/src/ssa/opt/die.rs
Original file line number Diff line number Diff line change
Expand Up @@ -377,13 +377,12 @@ fn handle_array_get_group(
next_out_of_bounds_index: &mut Option<usize>,
possible_index_out_of_bounds_indexes: &mut Vec<usize>,
) {
let Some(array_length) = function.dfg.try_get_array_length(*array) else {
if function.dfg.try_get_array_length(*array).is_none() {
// Nothing to do for slices
return;
};

let flattened_size = function.dfg.type_of_value(*array).flattened_size();
let element_size = flattened_size / array_length;
let element_size = function.dfg.type_of_value(*array).element_size();
if element_size <= 1 {
// Not a composite type
return;
Expand Down
40 changes: 40 additions & 0 deletions noir/noir-repo/compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs
Original file line number Diff line number Diff line change
Expand Up @@ -117,6 +117,10 @@ struct PerFunctionContext<'f> {
/// Track a value's last load across all blocks.
/// If a value is not used in anymore loads we can remove the last store to that value.
last_loads: HashMap<ValueId, (InstructionId, BasicBlockId)>,

/// Track whether the last instruction is an inc_rc/dec_rc instruction.
/// If it is we should not remove any known store values.
inside_rc_reload: bool,
}

impl<'f> PerFunctionContext<'f> {
Expand All @@ -131,6 +135,7 @@ impl<'f> PerFunctionContext<'f> {
blocks: BTreeMap::new(),
instructions_to_remove: BTreeSet::new(),
last_loads: HashMap::default(),
inside_rc_reload: false,
}
}

Expand Down Expand Up @@ -281,6 +286,21 @@ 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) {
aliases.insert(result);
} else {
references
.aliases
.insert(Expression::Other(result), AliasSet::known(result));
}
references.set_known_value(result, address);

self.last_loads.insert(address, (instruction, block_id));
}
}
Expand All @@ -296,6 +316,14 @@ impl<'f> PerFunctionContext<'f> {
self.instructions_to_remove.insert(*last_store);
}

let known_value = references.get_known_value(value);
if let Some(known_value) = known_value {
let known_value_is_address = known_value == address;
if known_value_is_address && !self.inside_rc_reload {
self.instructions_to_remove.insert(instruction);
}
}

references.set_known_value(address, value);
references.last_stores.insert(address, instruction);
}
Expand Down Expand Up @@ -350,6 +378,18 @@ impl<'f> PerFunctionContext<'f> {
Instruction::Call { arguments, .. } => self.mark_all_unknown(arguments, references),
_ => (),
}

self.track_rc_reload_state(instruction);
}

fn track_rc_reload_state(&mut self, instruction: InstructionId) {
match &self.inserter.function.dfg[instruction] {
// We just had an increment or decrement to an array's reference counter
Instruction::IncrementRc { .. } | Instruction::DecrementRc { .. } => {
self.inside_rc_reload = true;
}
_ => self.inside_rc_reload = false,
}
}

fn check_array_aliasing(&self, references: &mut Block, array: ValueId) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,9 @@ use acvm::acir::AcirField;

use crate::ssa::{
ir::{
basic_block::BasicBlockId, cfg::ControlFlowGraph, function::Function,
basic_block::BasicBlockId,
cfg::ControlFlowGraph,
function::{Function, RuntimeType},
instruction::TerminatorInstruction,
},
ssa_gen::Ssa,
Expand All @@ -30,7 +32,7 @@ impl Ssa {
/// 5. Replacing any jmpifs with constant conditions with jmps. If this causes the block to have
/// only 1 successor then (2) also will be applied.
///
/// Currently, 1 and 4 are unimplemented.
/// Currently, 1 is unimplemented.
#[tracing::instrument(level = "trace", skip(self))]
pub(crate) fn simplify_cfg(mut self) -> Self {
for function in self.functions.values_mut() {
Expand Down Expand Up @@ -72,6 +74,10 @@ fn simplify_function(function: &mut Function) {
// optimizations performed after this point on the same block should check if
// the inlining here was successful before continuing.
try_inline_into_predecessor(function, &mut cfg, block, predecessor);
} else {
drop(predecessors);

check_for_double_jmp(function, block, &mut cfg);
}
}
}
Expand Down Expand Up @@ -102,6 +108,71 @@ fn check_for_constant_jmpif(
}
}

/// Optimize a jmp to a block which immediately jmps elsewhere to just jmp to the second block.
fn check_for_double_jmp(function: &mut Function, block: BasicBlockId, cfg: &mut ControlFlowGraph) {
if matches!(function.runtime(), RuntimeType::Acir(_)) {
// We can't remove double jumps in ACIR functions as this interferes with the `flatten_cfg` pass.
return;
}

if !function.dfg[block].instructions().is_empty()
|| !function.dfg[block].parameters().is_empty()
{
return;
}

let Some(TerminatorInstruction::Jmp { destination: final_destination, arguments, .. }) =
function.dfg[block].terminator()
else {
return;
};

if !arguments.is_empty() {
return;
}

let final_destination = *final_destination;

let predecessors: Vec<_> = cfg.predecessors(block).collect();
for predecessor_block in predecessors {
let terminator_instruction = function.dfg[predecessor_block].take_terminator();
let redirected_terminator_instruction = match terminator_instruction {
TerminatorInstruction::JmpIf {
condition,
then_destination,
else_destination,
call_stack,
} => {
let then_destination =
if then_destination == block { final_destination } else { then_destination };
let else_destination =
if else_destination == block { final_destination } else { else_destination };
TerminatorInstruction::JmpIf {
condition,
then_destination,
else_destination,
call_stack,
}
}
TerminatorInstruction::Jmp { destination, arguments, call_stack } => {
assert_eq!(
destination, block,
"ICE: predecessor block doesn't jump to current block"
);
assert!(arguments.is_empty(), "ICE: predecessor jmp has arguments");
TerminatorInstruction::Jmp { destination: final_destination, arguments, call_stack }
}
TerminatorInstruction::Return { .. } => {
unreachable!("ICE: predecessor block should not have return terminator instruction")
}
};

function.dfg[predecessor_block].set_terminator(redirected_terminator_instruction);
cfg.recompute_block(function, predecessor_block);
}
cfg.recompute_block(function, block);
}

/// If the given block has block parameters, replace them with the jump arguments from the predecessor.
///
/// Currently, if this function is needed, `try_inline_into_predecessor` will also always apply,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -292,6 +292,7 @@ pub trait Recoverable {
#[derive(Debug, PartialEq, Eq, Clone)]
pub struct ModuleDeclaration {
pub ident: Ident,
pub outer_attributes: Vec<SecondaryAttribute>,
}

impl std::fmt::Display for ModuleDeclaration {
Expand Down
13 changes: 12 additions & 1 deletion noir/noir-repo/compiler/noirc_frontend/src/ast/visitor.rs
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ use crate::{
QuotedTypeId,
},
parser::{Item, ItemKind, ParsedSubModule},
token::Tokens,
token::{SecondaryAttribute, Tokens},
ParsedModule, QuotedType,
};

Expand Down Expand Up @@ -432,6 +432,8 @@ pub trait Visitor {
fn visit_struct_pattern(&mut self, _: &Path, _: &[(Ident, Pattern)], _: Span) -> bool {
true
}

fn visit_secondary_attribute(&mut self, _: &SecondaryAttribute, _: Span) {}
}

impl ParsedModule {
Expand Down Expand Up @@ -481,6 +483,9 @@ impl Item {
ItemKind::ModuleDecl(module_declaration) => {
module_declaration.accept(self.span, visitor);
}
ItemKind::InnerAttribute(attribute) => {
attribute.accept(self.span, visitor);
}
}
}
}
Expand Down Expand Up @@ -1289,6 +1294,12 @@ impl Pattern {
}
}

impl SecondaryAttribute {
pub fn accept(&self, span: Span, visitor: &mut impl Visitor) {
visitor.visit_secondary_attribute(self, span);
}
}

fn visit_expressions(expressions: &[Expression], visitor: &mut impl Visitor) {
for expression in expressions {
expression.accept(visitor);
Expand Down
Loading
Loading