Skip to content

Commit

Permalink
Merge branch 'master' into tf/simplify-after-is-unconstrained-resolved
Browse files Browse the repository at this point in the history
* master:
  chore: Add short circuit in ssa-gen for known if conditions (#7007)
  chore: Only resolved globals monomorphization (#7006)
  chore: Remove resolve_is_unconstrained pass (#7004)
  chore: require safety doc comment for unsafe instead of `//@safety` (#6992)
  • Loading branch information
TomAFrench committed Jan 9, 2025
2 parents 6c463dc + e0d6840 commit cf370f3
Show file tree
Hide file tree
Showing 81 changed files with 500 additions and 507 deletions.
2 changes: 0 additions & 2 deletions compiler/noirc_evaluator/src/ssa.rs
Original file line number Diff line number Diff line change
Expand Up @@ -153,8 +153,6 @@ fn optimize_all(builder: SsaBuilder, options: &SsaEvaluatorOptions) -> Result<Ss
.run_pass(Ssa::remove_unreachable_functions, "Removing Unreachable Functions")
.run_pass(Ssa::defunctionalize, "Defunctionalization")
.run_pass(Ssa::remove_paired_rc, "Removing Paired rc_inc & rc_decs")
.run_pass(Ssa::resolve_is_unconstrained, "Resolving IsUnconstrained")
.run_pass(Ssa::simplify_cfg, "Simplifying (1st)")
.run_pass(|ssa| ssa.inline_functions(options.inliner_aggressiveness), "Inlining (1st)")
// Run mem2reg with the CFG separated into blocks
.run_pass(Ssa::mem2reg, "Mem2Reg (1st)")
Expand Down
1 change: 1 addition & 0 deletions compiler/noirc_evaluator/src/ssa/ir/dfg.rs
Original file line number Diff line number Diff line change
Expand Up @@ -470,6 +470,7 @@ impl DataFlowGraph {

/// Remove an instruction by replacing it with a `Noop` instruction.
/// Doing this avoids shifting over each instruction after this one in its block's instructions vector.
#[allow(unused)]
pub(crate) fn remove_instruction(&mut self, instruction: InstructionId) {
self.instructions[instruction] = Instruction::Noop;
self.results.insert(instruction, smallvec::SmallVec::new());
Expand Down
5 changes: 4 additions & 1 deletion compiler/noirc_evaluator/src/ssa/ir/instruction/call.rs
Original file line number Diff line number Diff line change
Expand Up @@ -331,7 +331,10 @@ pub(super) fn simplify_call(
simplify_black_box_func(bb_func, arguments, dfg, block, call_stack)
}
Intrinsic::AsWitness => SimplifyResult::None,
Intrinsic::IsUnconstrained => SimplifyResult::None,
Intrinsic::IsUnconstrained => {
let result = dfg.runtime().is_brillig().into();
SimplifyResult::SimplifiedTo(dfg.make_constant(result, NumericType::bool()))
}
Intrinsic::DerivePedersenGenerators => {
if let Some(Type::Array(_, len)) = return_type.clone() {
simplify_derive_generators(dfg, arguments, len, block, call_stack)
Expand Down
16 changes: 7 additions & 9 deletions compiler/noirc_evaluator/src/ssa/opt/constant_folding.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1557,7 +1557,6 @@ mod test {
// After EnableSideEffectsIf removal:
brillig(inline) fn main f0 {
b0(v0: Field, v1: Field, v2: u1):
v4 = call is_unconstrained() -> u1
v7 = call to_be_radix(v0, u32 256) -> [u8; 1] // `a.to_be_radix(256)`;
inc_rc v7
v8 = call to_be_radix(v0, u32 256) -> [u8; 1] // duplicate load of `a`
Expand All @@ -1574,14 +1573,13 @@ mod test {
let expected = "
brillig(inline) fn main f0 {
b0(v0: Field, v1: Field, v2: u1):
v4 = call is_unconstrained() -> u1
v7 = call to_be_radix(v0, u32 256) -> [u8; 1]
inc_rc v7
inc_rc v7
v8 = cast v2 as Field
v9 = mul v0, v8
v10 = call to_be_radix(v9, u32 256) -> [u8; 1]
inc_rc v10
v5 = call to_be_radix(v0, u32 256) -> [u8; 1]
inc_rc v5
inc_rc v5
v6 = cast v2 as Field
v7 = mul v0, v6
v8 = call to_be_radix(v7, u32 256) -> [u8; 1]
inc_rc v8
enable_side_effects v2
return
}
Expand Down
1 change: 0 additions & 1 deletion compiler/noirc_evaluator/src/ssa/opt/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,6 @@ mod remove_bit_shifts;
mod remove_enable_side_effects;
mod remove_if_else;
mod remove_unreachable;
mod resolve_is_unconstrained;
mod simplify_cfg;
mod unrolling;

Expand Down
55 changes: 0 additions & 55 deletions compiler/noirc_evaluator/src/ssa/opt/resolve_is_unconstrained.rs

This file was deleted.

23 changes: 23 additions & 0 deletions compiler/noirc_evaluator/src/ssa/ssa_gen/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ pub(crate) mod context;
mod program;
mod value;

use acvm::AcirField;
use noirc_frontend::token::FmtStrFragment;
pub(crate) use program::Ssa;

Expand Down Expand Up @@ -595,6 +596,9 @@ impl<'a> FunctionContext<'a> {
/// ```
fn codegen_if(&mut self, if_expr: &ast::If) -> Result<Values, RuntimeError> {
let condition = self.codegen_non_tuple_expression(&if_expr.condition)?;
if let Some(result) = self.try_codegen_constant_if(condition, if_expr) {
return result;
}

let then_block = self.builder.insert_block();
let else_block = self.builder.insert_block();
Expand Down Expand Up @@ -633,6 +637,25 @@ impl<'a> FunctionContext<'a> {
Ok(result)
}

/// If the condition is known, skip codegen for the then/else branch and only compile the
/// relevant branch.
fn try_codegen_constant_if(
&mut self,
condition: ValueId,
if_expr: &ast::If,
) -> Option<Result<Values, RuntimeError>> {
let condition = self.builder.current_function.dfg.get_numeric_constant(condition)?;

Some(if condition.is_zero() {
match if_expr.alternative.as_ref() {
Some(alternative) => self.codegen_expression(alternative),
None => Ok(Self::unit_value()),
}
} else {
self.codegen_expression(&if_expr.consequence)
})
}

fn codegen_tuple(&mut self, tuple: &[Expression]) -> Result<Values, RuntimeError> {
Ok(Tree::Branch(try_vecmap(tuple, |expr| self.codegen_expression(expr))?))
}
Expand Down
12 changes: 6 additions & 6 deletions compiler/noirc_frontend/src/debug/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -523,8 +523,8 @@ pub fn build_debug_crate_file() -> String {
__debug_var_assign_oracle(var_id, value);
}
pub fn __debug_var_assign<T>(var_id: u32, value: T) {
/// Safety: debug context
unsafe {
//@safety: debug context
{
__debug_var_assign_inner(var_id, value);
}}
Expand All @@ -536,8 +536,8 @@ pub fn build_debug_crate_file() -> String {
__debug_var_drop_oracle(var_id);
}
pub fn __debug_var_drop(var_id: u32) {
/// Safety: debug context
unsafe {
//@safety: debug context
{
__debug_var_drop_inner(var_id);
}}
Expand All @@ -549,8 +549,8 @@ pub fn build_debug_crate_file() -> String {
__debug_fn_enter_oracle(fn_id);
}
pub fn __debug_fn_enter(fn_id: u32) {
/// Safety: debug context
unsafe {
//@safety: debug context
{
__debug_fn_enter_inner(fn_id);
}}
Expand All @@ -562,8 +562,8 @@ pub fn build_debug_crate_file() -> String {
__debug_fn_exit_oracle(fn_id);
}
pub fn __debug_fn_exit(fn_id: u32) {
/// Safety: debug context
unsafe {
//@safety: debug context
{
__debug_fn_exit_inner(fn_id);
}}
Expand All @@ -575,8 +575,8 @@ pub fn build_debug_crate_file() -> String {
__debug_dereference_assign_oracle(var_id, value);
}
pub fn __debug_dereference_assign<T>(var_id: u32, value: T) {
/// Safety: debug context
unsafe {
//@safety: debug context
{
__debug_dereference_assign_inner(var_id, value);
}}
Expand All @@ -603,8 +603,8 @@ pub fn build_debug_crate_file() -> String {
__debug_oracle_member_assign_{n}(var_id, value, {vars});
}}
pub fn __debug_member_assign_{n}<T, Index>(var_id: u32, value: T, {var_sig}) {{
/// Safety: debug context
unsafe {{
//@safety: debug context
__debug_inner_member_assign_{n}(var_id, value, {vars});
}}
}}
Expand Down
4 changes: 2 additions & 2 deletions compiler/noirc_frontend/src/elaborator/expressions.rs
Original file line number Diff line number Diff line change
Expand Up @@ -58,8 +58,8 @@ impl<'context> Elaborator<'context> {
ExpressionKind::Comptime(comptime, _) => {
return self.elaborate_comptime_block(comptime, expr.span)
}
ExpressionKind::Unsafe(block_expression, _) => {
self.elaborate_unsafe_block(block_expression, expr.span)
ExpressionKind::Unsafe(block_expression, span) => {
self.elaborate_unsafe_block(block_expression, span)
}
ExpressionKind::Resolved(id) => return (id, self.interner.id_type(id)),
ExpressionKind::Interned(id) => {
Expand Down
22 changes: 3 additions & 19 deletions compiler/noirc_frontend/src/lexer/lexer.rs
Original file line number Diff line number Diff line change
Expand Up @@ -726,7 +726,7 @@ impl<'a> Lexer<'a> {
}

fn parse_comment(&mut self, start: u32) -> SpannedTokenResult {
let mut doc_style = match self.peek_char() {
let doc_style = match self.peek_char() {
Some('!') => {
self.next_char();
Some(DocStyle::Inner)
Expand All @@ -735,17 +735,9 @@ impl<'a> Lexer<'a> {
self.next_char();
Some(DocStyle::Outer)
}
Some('@') => Some(DocStyle::Safety),
_ => None,
};
let mut comment = self.eat_while(None, |ch| ch != '\n');
if doc_style == Some(DocStyle::Safety) {
if comment.starts_with("@safety") {
comment = comment["@safety".len()..].to_string();
} else {
doc_style = None;
}
}
let comment = self.eat_while(None, |ch| ch != '\n');

if !comment.is_ascii() {
let span = Span::from(start..self.position);
Expand All @@ -760,7 +752,7 @@ impl<'a> Lexer<'a> {
}

fn parse_block_comment(&mut self, start: u32) -> SpannedTokenResult {
let mut doc_style = match self.peek_char() {
let doc_style = match self.peek_char() {
Some('!') => {
self.next_char();
Some(DocStyle::Inner)
Expand All @@ -769,7 +761,6 @@ impl<'a> Lexer<'a> {
self.next_char();
Some(DocStyle::Outer)
}
Some('@') => Some(DocStyle::Safety),
_ => None,
};

Expand All @@ -796,13 +787,6 @@ impl<'a> Lexer<'a> {
ch => content.push(ch),
}
}
if doc_style == Some(DocStyle::Safety) {
if content.starts_with("@safety") {
content = content["@safety".len()..].to_string();
} else {
doc_style = None;
}
}
if depth == 0 {
if !content.is_ascii() {
let span = Span::from(start..self.position);
Expand Down
3 changes: 0 additions & 3 deletions compiler/noirc_frontend/src/lexer/token.rs
Original file line number Diff line number Diff line change
Expand Up @@ -345,7 +345,6 @@ impl Display for FmtStrFragment {
pub enum DocStyle {
Outer,
Inner,
Safety,
}

#[derive(Debug, Clone, PartialEq, Eq, Hash, PartialOrd, Ord)]
Expand Down Expand Up @@ -425,13 +424,11 @@ impl fmt::Display for Token {
Token::LineComment(ref s, style) => match style {
Some(DocStyle::Inner) => write!(f, "//!{s}"),
Some(DocStyle::Outer) => write!(f, "///{s}"),
Some(DocStyle::Safety) => write!(f, "//@safety{s}"),
None => write!(f, "//{s}"),
},
Token::BlockComment(ref s, style) => match style {
Some(DocStyle::Inner) => write!(f, "/*!{s}*/"),
Some(DocStyle::Outer) => write!(f, "/**{s}*/"),
Some(DocStyle::Safety) => write!(f, "/*@safety{s}*/"),
None => write!(f, "/*{s}*/"),
},
Token::Quote(ref stream) => {
Expand Down
6 changes: 2 additions & 4 deletions compiler/noirc_frontend/src/monomorphization/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -934,11 +934,9 @@ impl<'interner> Monomorphizer<'interner> {
.into_hir_expression(self.interner, global.location)
.map_err(MonomorphizationError::InterpreterError)?
} else {
let let_ = self.interner.get_global_let_statement(*global_id).expect(
"Globals should have a corresponding let statement by monomorphization",
);
let_.expression
unreachable!("All global values should be resolved at compile time and before monomorphization");
};

self.expr(expr)?
}
DefinitionKind::Local(_) => match self.lookup_captured_expr(ident.id) {
Expand Down
16 changes: 13 additions & 3 deletions compiler/noirc_frontend/src/parser/errors.rs
Original file line number Diff line number Diff line change
Expand Up @@ -110,8 +110,10 @@ pub enum ParserErrorReason {
WrongNumberOfAttributeArguments { name: String, min: usize, max: usize, found: usize },
#[error("The `deprecated` attribute expects a string argument")]
DeprecatedAttributeExpectsAStringArgument,
#[error("Unsafe block must start with a safety comment")]
#[error("Unsafe block must have a safety doc comment above it")]
MissingSafetyComment,
#[error("Unsafe block must start with a safety comment")]
UnsafeDocCommentDoesNotStartWithSafety,
#[error("Missing parameters for function definition")]
MissingParametersForFunctionDefinition,
}
Expand Down Expand Up @@ -283,10 +285,18 @@ impl<'a> From<&'a ParserError> for Diagnostic {
error.span,
),
ParserErrorReason::MissingSafetyComment => Diagnostic::simple_warning(
"Missing Safety Comment".into(),
"Unsafe block must start with a safety comment: //@safety".into(),
"Unsafe block must have a safety doc comment above it".into(),
"The doc comment must start with the \"Safety: \" word".into(),
error.span,
),
ParserErrorReason::UnsafeDocCommentDoesNotStartWithSafety => {
Diagnostic::simple_warning(
"Unsafe block must start with a safety comment".into(),
"The doc comment above this unsafe block must start with the \"Safety: \" word"
.into(),
error.span,
)
}
ParserErrorReason::MissingParametersForFunctionDefinition => {
Diagnostic::simple_error(
"Missing parameters for function definition".into(),
Expand Down
Loading

0 comments on commit cf370f3

Please sign in to comment.