Skip to content

Commit

Permalink
Don't emit unchecked operations for Fields, or when multiplying bools
Browse files Browse the repository at this point in the history
  • Loading branch information
asterite committed Jan 10, 2025
1 parent f34fb51 commit e0ff78f
Show file tree
Hide file tree
Showing 5 changed files with 81 additions and 40 deletions.
8 changes: 0 additions & 8 deletions compiler/noirc_evaluator/src/ssa/function_builder/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -251,14 +251,6 @@ impl FunctionBuilder {
operator: BinaryOp,
rhs: ValueId,
) -> ValueId {
let lhs_type = self.type_of_value(lhs);
let rhs_type = self.type_of_value(rhs);
if operator != BinaryOp::Shl && operator != BinaryOp::Shr {
assert_eq!(
lhs_type, rhs_type,
"ICE - Binary instruction operands must have the same type"
);
}
let instruction = Instruction::Binary(Binary { lhs, rhs, operator });
self.insert_instruction(instruction, None).first()
}
Expand Down
41 changes: 40 additions & 1 deletion compiler/noirc_evaluator/src/ssa/ir/dfg.rs
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,8 @@ use super::{
call_stack::{CallStack, CallStackHelper, CallStackId},
function::{FunctionId, RuntimeType},
instruction::{
Instruction, InstructionId, InstructionResultType, Intrinsic, TerminatorInstruction,
Binary, BinaryOp, Instruction, InstructionId, InstructionResultType, Intrinsic,
TerminatorInstruction,
},
map::DenseMap,
types::{NumericType, Type},
Expand Down Expand Up @@ -174,6 +175,44 @@ impl DataFlowGraph {
instruction_data: Instruction,
ctrl_typevars: Option<Vec<Type>>,
) -> InstructionId {
let instruction_data = if let Instruction::Binary(binary) = instruction_data {
let lhs = binary.lhs;
let rhs = binary.rhs;
let operator = binary.operator;
let lhs_type = self.type_of_value(lhs);
let rhs_type = self.type_of_value(rhs);
if operator != BinaryOp::Shl && operator != BinaryOp::Shr {
assert_eq!(
lhs_type, rhs_type,
"ICE - Binary instruction operands must have the same type"
);
}

let operator = if lhs_type.is_field() {
// Unchecked operations between fields or bools don't make sense, so we convert those to non-unchecked

Check warning on line 192 in compiler/noirc_evaluator/src/ssa/ir/dfg.rs

View workflow job for this annotation

GitHub Actions / Code

Unknown word (bools)
// to reduce noise and confusion in the generated SSA.
match operator {
BinaryOp::Add { unchecked: true } => BinaryOp::Add { unchecked: false },
BinaryOp::Sub { unchecked: true } => BinaryOp::Sub { unchecked: false },
BinaryOp::Mul { unchecked: true } => BinaryOp::Mul { unchecked: false },
_ => operator,
}
} else if lhs_type.is_bool() {
// Unchecked mul between bools doesn't make sense, so we convert that to non-unchecked

Check warning on line 201 in compiler/noirc_evaluator/src/ssa/ir/dfg.rs

View workflow job for this annotation

GitHub Actions / Code

Unknown word (bools)
if let BinaryOp::Mul { unchecked: true } = operator {
BinaryOp::Mul { unchecked: false }
} else {
operator
}
} else {
operator
};

Instruction::Binary(Binary { operator, ..binary })
} else {
instruction_data
};

let id = self.instructions.insert(instruction_data);
self.make_instruction_results(id, ctrl_typevars);
id
Expand Down
10 changes: 10 additions & 0 deletions compiler/noirc_evaluator/src/ssa/ir/types.rs
Original file line number Diff line number Diff line change
Expand Up @@ -119,6 +119,16 @@ impl Type {
matches!(self, Type::Numeric(NumericType::Unsigned { .. }))
}

/// Returns whether the `Type` represents the Field type.
pub(crate) fn is_field(&self) -> bool {
matches!(self, Type::Numeric(NumericType::NativeField))
}

/// Returns whether the `Type` is `u1`
pub(crate) fn is_bool(&self) -> bool {
matches!(self, Type::Numeric(NumericType::Unsigned { bit_size: 1 }))
}

/// Create a new signed integer type with the given amount of bits.
pub(crate) fn signed(bit_size: u32) -> Type {
Type::Numeric(NumericType::Signed { bit_size })
Expand Down
60 changes: 30 additions & 30 deletions compiler/noirc_evaluator/src/ssa/opt/flatten_cfg.rs
Original file line number Diff line number Diff line change
Expand Up @@ -850,9 +850,9 @@ mod test {
enable_side_effects u1 1
v3 = cast v0 as Field
v4 = cast v1 as Field
v6 = unchecked_mul v3, Field 3
v8 = unchecked_mul v4, Field 4
v9 = unchecked_add v6, v8
v6 = mul v3, Field 3
v8 = mul v4, Field 4
v9 = add v6, v8
return v9
}
";
Expand Down Expand Up @@ -881,7 +881,7 @@ mod test {
acir(inline) fn main f0 {
b0(v0: u1, v1: u1):
enable_side_effects v0
v2 = unchecked_mul v1, v0
v2 = mul v1, v0
constrain v2 == v0
v3 = not v0
enable_side_effects u1 1
Expand Down Expand Up @@ -916,9 +916,9 @@ mod test {
v3 = not v0
v4 = cast v0 as Field
v5 = cast v3 as Field
v7 = unchecked_mul v4, Field 5
v8 = unchecked_mul v5, v2
v9 = unchecked_add v7, v8
v7 = mul v4, Field 5
v8 = mul v5, v2
v9 = add v7, v8
store v9 at v1
enable_side_effects u1 1
return
Expand Down Expand Up @@ -954,17 +954,17 @@ mod test {
v3 = not v0
v4 = cast v0 as Field
v5 = cast v3 as Field
v7 = unchecked_mul v4, Field 5
v8 = unchecked_mul v5, v2
v9 = unchecked_add v7, v8
v7 = mul v4, Field 5
v8 = mul v5, v2
v9 = add v7, v8
store v9 at v1
enable_side_effects v3
v10 = load v1 -> Field
v11 = cast v3 as Field
v12 = cast v0 as Field
v14 = unchecked_mul v11, Field 6
v15 = unchecked_mul v12, v10
v16 = unchecked_add v14, v15
v14 = mul v11, Field 6
v15 = mul v12, v10
v16 = add v14, v15
store v16 at v1
enable_side_effects u1 1
return
Expand Down Expand Up @@ -1066,32 +1066,32 @@ mod test {
v3 = not v0
v4 = cast v0 as Field
v5 = cast v3 as Field
v7 = unchecked_mul v4, Field 2
v8 = unchecked_add v7, v5
v9 = unchecked_mul v0, v1
v7 = mul v4, Field 2
v8 = add v7, v5
v9 = mul v0, v1
enable_side_effects v9
v10 = not v9
v11 = cast v9 as Field
v12 = cast v10 as Field
v14 = unchecked_mul v11, Field 5
v15 = unchecked_mul v12, v8
v16 = unchecked_add v14, v15
v14 = mul v11, Field 5
v15 = mul v12, v8
v16 = add v14, v15
v17 = not v1
v18 = unchecked_mul v0, v17
v18 = mul v0, v17
enable_side_effects v18
v19 = not v18
v20 = cast v18 as Field
v21 = cast v19 as Field
v23 = unchecked_mul v20, Field 6
v24 = unchecked_mul v21, v16
v25 = unchecked_add v23, v24
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
v29 = unchecked_mul v26, Field 3
v30 = unchecked_mul v27, v25
v31 = unchecked_add v29, v30
v29 = mul v26, Field 3
v30 = mul v27, v25
v31 = add v29, v30
enable_side_effects u1 1
return v31
}";
Expand Down Expand Up @@ -1446,9 +1446,9 @@ mod test {
v2 = not v0
v3 = cast v0 as Field
v4 = cast v2 as Field
v6 = unchecked_mul v3, Field 2
v7 = unchecked_mul v4, v3
v8 = unchecked_add v6, v7
v6 = mul v3, Field 2
v7 = mul v4, v3
v8 = add v6, v7
enable_side_effects u1 1
return v8
}
Expand Down Expand Up @@ -1491,7 +1491,7 @@ mod test {
b0(v0: u1, v1: u1):
enable_side_effects v0
v2 = cast v0 as Field
v4 = unchecked_mul v2, Field 2
v4 = mul v2, Field 2
v5 = make_array [v4] : [Field; 1]
enable_side_effects u1 1
return v5
Expand Down
2 changes: 1 addition & 1 deletion compiler/noirc_evaluator/src/ssa/parser/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -346,7 +346,7 @@ fn test_binary() {
let src = format!(
"
acir(inline) fn main f0 {{
b0(v0: Field, v1: Field):
b0(v0: u32, v1: u32):
v2 = {op} v0, v1
return
}}
Expand Down

0 comments on commit e0ff78f

Please sign in to comment.