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: separate slice and array types in the AST + as_slice builtin #4550

Closed
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
41 commits
Select commit Hold shift + click to select a range
6951dfb
wip separating out array and slice types in the AST
michaeljklein Mar 6, 2024
2f93bed
Update ordering of Type::Array construction in type_check/expr.rs
jfecher Mar 7, 2024
654732a
wip debugging and updating tests: fix parser and parser tests for amp…
michaeljklein Mar 8, 2024
23d3a72
wip debugging: test slice index, got slice index working, slice-speci…
michaeljklein Mar 8, 2024
c683028
got tests passing, update stdlib methods to take slices, fix missing …
michaeljklein Mar 11, 2024
a571593
Merge branch 'master' into michaeljklein/separate-slice-ast
michaeljklein Mar 11, 2024
6473805
update poseidon/mimic hash slice literals, add eq/ord/default instanc…
michaeljklein Mar 11, 2024
4f66228
added missing element_type_at_index case for slices, fixed all but on…
michaeljklein Mar 11, 2024
cd2807d
update slice literal in pedersen_hash wasm example
michaeljklein Mar 11, 2024
7a50955
remove redundant match on array vs slice
michaeljklein Mar 11, 2024
592b94d
Merge branch 'master' into michaeljklein/separate-slice-ast
michaeljklein Mar 11, 2024
4adc691
remove unused variable, flip result type order
michaeljklein Mar 11, 2024
d986bfc
add as_slice builtin function, add execution test
michaeljklein Mar 11, 2024
462af64
wip debugging test failures, add dynamic array case to test
michaeljklein Mar 11, 2024
2b749e1
wip debugging error on valid index of result of as_slice
michaeljklein Mar 11, 2024
3da8474
update can_omit_element_sizes_array
vezenovm Mar 11, 2024
197fd77
remove dbg calls and cleanup, add brillig implementation for AsSlice,…
michaeljklein Mar 11, 2024
a5f7d9e
wip debugging brillig as_slice implementation, rewrote implementation…
michaeljklein Mar 12, 2024
e8637a5
wip debugging brillig slice access error, fixed wrong initialization …
michaeljklein Mar 12, 2024
efa81db
wip debugging: cleanup for second pair of eyes
michaeljklein Mar 12, 2024
84cfbc3
got brillig tests passing using version of convert_ssa_array_set, cle…
michaeljklein Mar 12, 2024
c29add0
remove debugging return
michaeljklein Mar 12, 2024
5be64bd
Merge branch 'master' into michaeljklein/as-slice-builtin
michaeljklein Mar 12, 2024
fecda34
cargo clippy / fmt
michaeljklein Mar 12, 2024
b6feea0
fix destination length init in brillig as_slice implementation, split…
michaeljklein Mar 13, 2024
e940103
Merge branch 'master' into michaeljklein/as-slice-builtin
michaeljklein Mar 13, 2024
269bdda
combine convert_ssa_as_slice with convert_ssa_array_set to pull in re…
michaeljklein Mar 13, 2024
e5da158
Merge branch 'master' into michaeljklein/as-slice-builtin
michaeljklein Mar 13, 2024
f66772b
move as_slice logic out of convert_ssa_array_set
michaeljklein Mar 13, 2024
cad7214
Merge branch 'master' into michaeljklein/separate-slice-ast
michaeljklein Mar 13, 2024
7eb7b76
keep new builtin as_slice
michaeljklein Mar 13, 2024
243fcdc
fix slice literals from merging master
michaeljklein Mar 13, 2024
22e797f
Merge branch 'master' into michaeljklein/separate-slice-ast-builtin-a…
michaeljklein Mar 13, 2024
74b5250
add try_to_usize to complete roundtrip of usize -> Field and back for…
michaeljklein Mar 14, 2024
ba7a183
Merge branch 'master' into michaeljklein/separate-slice-ast-builtin-a…
michaeljklein Mar 14, 2024
5e9861d
replace arrays with slice literals in Hash impl's, add distinct slice…
michaeljklein Mar 14, 2024
1968152
Merge branch 'master' into michaeljklein/separate-slice-ast-builtin-a…
michaeljklein Mar 14, 2024
60074fd
Merge branch 'master' into michaeljklein/separate-slice-ast-builtin-a…
michaeljklein Mar 15, 2024
8d4c81a
Merge branch 'master' into michaeljklein/separate-slice-ast-builtin-a…
michaeljklein Mar 15, 2024
4fbd911
Merge branch 'master' into michaeljklein/separate-slice-ast-builtin-a…
michaeljklein Mar 15, 2024
e18f47b
remove unused PartialEq
michaeljklein Mar 15, 2024
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
5 changes: 5 additions & 0 deletions acvm-repo/acir_field/src/generic_ark.rs
Original file line number Diff line number Diff line change
Expand Up @@ -254,6 +254,11 @@ impl<F: PrimeField> FieldElement<F> {
(self.num_bits() <= 64).then(|| self.to_u128() as u64)
}

pub fn try_to_usize(&self) -> Option<usize> {
let usize_bits: u32 = std::mem::size_of::<usize>().try_into().ok()?;
(self.num_bits() <= usize_bits).then(|| self.to_u128() as usize)
}

/// Computes the inverse or returns zero if the inverse does not exist
/// Before using this FieldElement, please ensure that this behavior is necessary
pub fn inverse(&self) -> FieldElement<F> {
Expand Down
69 changes: 51 additions & 18 deletions compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs
Original file line number Diff line number Diff line change
Expand Up @@ -453,6 +453,33 @@ impl<'block> BrilligBlock<'block> {
self.convert_ssa_array_len(arguments[0], result_variable.address, dfg);
}
}
Value::Intrinsic(Intrinsic::AsSlice) => {
let source_variable = self.convert_ssa_value(arguments[0], dfg);
let result_ids = dfg.instruction_results(instruction_id);
let destination_len_variable = self.variables.define_single_addr_variable(
self.function_context,
self.brillig_context,
result_ids[0],
dfg,
);
let destination_variable = self.variables.define_variable(
self.function_context,
self.brillig_context,
result_ids[1],
dfg,
);
let source_size_as_register = self.convert_ssa_array_set(
source_variable,
destination_variable,
None,
"as_slice".to_string(),
);

// we need to explicitly set the destination_len_variable
self.brillig_context
.mov_instruction(destination_len_variable.address, source_size_as_register);
self.brillig_context.deallocate_register(source_size_as_register);
}
Value::Intrinsic(
Intrinsic::SlicePushBack
| Intrinsic::SlicePopBack
Expand Down Expand Up @@ -612,13 +639,13 @@ impl<'block> BrilligBlock<'block> {
dfg,
);
self.validate_array_index(source_variable, index_register);

self.convert_ssa_array_set(
let source_size_as_register = self.convert_ssa_array_set(
source_variable,
destination_variable,
index_register.address,
value_variable,
Some((index_register.address, value_variable)),
"array set".to_string(),
);
self.brillig_context.deallocate_register(source_size_as_register);
}
Instruction::RangeCheck { value, max_bit_size, assert_message } => {
let value = self.convert_ssa_single_addr_value(*value, dfg);
Expand Down Expand Up @@ -806,23 +833,26 @@ impl<'block> BrilligBlock<'block> {

/// Array set operation in SSA returns a new array or slice that is a copy of the parameter array or slice
/// With a specific value changed.
///
/// Returns `source_size_as_register`, which is expected to be deallocated with:
/// `self.brillig_context.deallocate_register(source_size_as_register)`
fn convert_ssa_array_set(
&mut self,
source_variable: BrilligVariable,
destination_variable: BrilligVariable,
index_register: MemoryAddress,
value_variable: BrilligVariable,
) {
opt_index_and_value: Option<(MemoryAddress, BrilligVariable)>,
method_str: String,
) -> MemoryAddress {
let destination_pointer = match destination_variable {
BrilligVariable::BrilligArray(BrilligArray { pointer, .. }) => pointer,
BrilligVariable::BrilligVector(BrilligVector { pointer, .. }) => pointer,
_ => unreachable!("ICE: array set returns non-array"),
_ => unreachable!("ICE: {method_str} returns non-array"),
};

let reference_count = match source_variable {
BrilligVariable::BrilligArray(BrilligArray { rc, .. })
| BrilligVariable::BrilligVector(BrilligVector { rc, .. }) => rc,
_ => unreachable!("ICE: array set on non-array"),
_ => unreachable!("ICE: {method_str} on non-array"),
};

let (source_pointer, source_size_as_register) = match source_variable {
Expand All @@ -836,7 +866,7 @@ impl<'block> BrilligBlock<'block> {
self.brillig_context.mov_instruction(source_size_register, size);
(pointer, source_size_register)
}
_ => unreachable!("ICE: array set on non-array"),
_ => unreachable!("ICE: {method_str} on non-array"),
};

// Here we want to compare the reference count against 1.
Expand Down Expand Up @@ -879,18 +909,20 @@ impl<'block> BrilligBlock<'block> {
self.brillig_context.mov_instruction(target_size, source_size_as_register);
self.brillig_context.usize_const(target_rc, 1_usize.into());
}
_ => unreachable!("ICE: array set on non-array"),
_ => unreachable!("ICE: {method_str} on non-array"),
}

// Then set the value in the newly created array
self.store_variable_in_array(
destination_pointer,
SingleAddrVariable::new_usize(index_register),
value_variable,
);
if let Some((index_register, value_variable)) = opt_index_and_value {
// Then set the value in the newly created array
self.store_variable_in_array(
destination_pointer,
SingleAddrVariable::new_usize(index_register),
value_variable,
);
}

self.brillig_context.deallocate_register(source_size_as_register);
self.brillig_context.deallocate_register(condition);
source_size_as_register
}

pub(crate) fn store_variable_in_array_with_ctx(
Expand Down Expand Up @@ -1319,6 +1351,7 @@ impl<'block> BrilligBlock<'block> {
Value::Param { .. } | Value::Instruction { .. } => {
// All block parameters and instruction results should have already been
// converted to registers so we fetch from the cache.

self.variables.get_allocation(self.function_context, value_id, dfg)
}
Value::NumericConstant { constant, .. } => {
Expand Down
77 changes: 73 additions & 4 deletions compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1025,7 +1025,13 @@ impl Context {
self.array_set_value(&store_value, result_block_id, &mut var_index)?;

let element_type_sizes = if !can_omit_element_sizes_array(&array_typ) {
Some(self.init_element_type_sizes_array(&array_typ, array_id, None, dfg)?)
let acir_value = self.convert_value(array_id, dfg);
Some(self.init_element_type_sizes_array(
&array_typ,
array_id,
Some(&acir_value),
dfg,
)?)
} else {
None
};
Expand Down Expand Up @@ -1246,7 +1252,8 @@ impl Context {
let read = self.acir_context.read_from_memory(source, &index_var)?;
Ok::<AcirValue, RuntimeError>(AcirValue::Var(read, AcirType::field()))
})?;
self.initialize_array(destination, array_len, Some(AcirValue::Array(init_values.into())))?;
let array: im::Vector<AcirValue> = init_values.into();
self.initialize_array(destination, array_len, Some(AcirValue::Array(array.clone())))?;
Ok(())
}

Expand Down Expand Up @@ -1656,13 +1663,75 @@ impl Context {
self.acir_context.bit_decompose(endian, field, bit_size, result_type)
}
Intrinsic::ArrayLen => {
let len = match self.convert_value(arguments[0], dfg) {
AcirValue::Var(_, _) => unreachable!("Non-array passed to array.len() method"),
let converted = self.convert_value(arguments[0], dfg);
let len = match converted {
AcirValue::Var(acir_var, acir_type) => {
if Type::Numeric(acir_type.to_numeric_type()) != Type::length_type() {
unreachable!(
"ICE: array has length of unexpected type: {:?}",
acir_type
);
}

if !self.acir_context.is_constant(&acir_var) {
unreachable!("ICE: array has non-constant length: {:?}", acir_var);
}

self.acir_context
.constant(acir_var)
.try_to_usize()
.expect("ICE: array with length > usize::MAX")
}
AcirValue::Array(values) => values.len(),
AcirValue::DynamicArray(array) => array.len,
};
Ok(vec![AcirValue::Var(self.acir_context.add_constant(len), AcirType::field())])
}
Intrinsic::AsSlice => {
let (slice_contents, slice_typ, block_id) =
self.check_array_is_initialized(arguments[0], dfg)?;
assert!(!slice_typ.is_nested_slice(), "ICE: Nested slice used in ACIR generation");

let result_block_id = self.block_id(&result_ids[1]);
let acir_value = self.convert_value(slice_contents, dfg);

let array_len = if !slice_typ.contains_slice_element() {
slice_typ.flattened_size()
} else {
self.flattened_slice_size(slice_contents, dfg)
};
let slice_length = self.acir_context.add_constant(array_len);
self.copy_dynamic_array(block_id, result_block_id, array_len)?;

let element_type_sizes = if !can_omit_element_sizes_array(&slice_typ) {
Some(self.init_element_type_sizes_array(
&slice_typ,
slice_contents,
Some(&acir_value),
dfg,
)?)
} else {
None
};

let value_types = self.convert_value(slice_contents, dfg).flat_numeric_types();
let result_len = value_types.len();
assert!(
array_len == result_len,
"AsSlice: unexpected length difference: {:?} != {:?}",
array_len,
value_types.len()
);

let result = AcirValue::DynamicArray(AcirDynamicArray {
block_id: result_block_id,
len: result_len,
value_types,
element_type_sizes,
});

Ok(vec![AcirValue::Var(slice_length, AcirType::field()), result])
}
Intrinsic::SlicePushBack => {
// arguments = [slice_length, slice_contents, ...elements_to_push]
let slice_length = self.convert_value(arguments[0], dfg).into_var()?;
Expand Down
4 changes: 4 additions & 0 deletions compiler/noirc_evaluator/src/ssa/ir/instruction.rs
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ pub(crate) type InstructionId = Id<Instruction>;
#[derive(Debug, Copy, Clone, PartialEq, Eq, Hash)]
pub(crate) enum Intrinsic {
ArrayLen,
AsSlice,
AssertConstant,
SlicePushBack,
SlicePushFront,
Expand All @@ -57,6 +58,7 @@ impl std::fmt::Display for Intrinsic {
fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
match self {
Intrinsic::ArrayLen => write!(f, "array_len"),
Intrinsic::AsSlice => write!(f, "as_slice"),
Intrinsic::AssertConstant => write!(f, "assert_constant"),
Intrinsic::SlicePushBack => write!(f, "slice_push_back"),
Intrinsic::SlicePushFront => write!(f, "slice_push_front"),
Expand Down Expand Up @@ -89,6 +91,7 @@ impl Intrinsic {
Intrinsic::ToBits(_) | Intrinsic::ToRadix(_) => true,

Intrinsic::ArrayLen
| Intrinsic::AsSlice
| Intrinsic::SlicePushBack
| Intrinsic::SlicePushFront
| Intrinsic::SlicePopBack
Expand All @@ -109,6 +112,7 @@ impl Intrinsic {
pub(crate) fn lookup(name: &str) -> Option<Intrinsic> {
match name {
"array_len" => Some(Intrinsic::ArrayLen),
"as_slice" => Some(Intrinsic::AsSlice),
"assert_constant" => Some(Intrinsic::AssertConstant),
"apply_range_constraint" => Some(Intrinsic::ApplyRangeConstraint),
"slice_push_back" => Some(Intrinsic::SlicePushBack),
Expand Down
15 changes: 15 additions & 0 deletions compiler/noirc_evaluator/src/ssa/ir/instruction/call.rs
Original file line number Diff line number Diff line change
Expand Up @@ -84,6 +84,21 @@ pub(super) fn simplify_call(
SimplifyResult::None
}
}
Intrinsic::AsSlice => {
let slice = dfg.get_array_constant(arguments[0]);
if let Some((slice, element_type)) = slice {
let new_element_type = match element_type {
Type::Array(array_typ, _) => Type::Slice(array_typ),
Type::Slice(slice_typ) => Type::Slice(slice_typ),
_ => unreachable!("ICE: non-array passed to as_slice"),
};
let slice_length = dfg.make_constant(slice.len().into(), Type::length_type());
let new_slice = dfg.make_array(slice, new_element_type);
SimplifyResult::SimplifiedToMultiple(vec![slice_length, new_slice])
} else {
SimplifyResult::None
}
}
Intrinsic::SlicePushBack => {
let slice = dfg.get_array_constant(arguments[1]);
if let Some((mut slice, element_type)) = slice {
Expand Down
9 changes: 7 additions & 2 deletions compiler/noirc_evaluator/src/ssa/ir/printer.rs
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ use super::{
basic_block::BasicBlockId,
function::Function,
instruction::{ConstrainError, Instruction, InstructionId, TerminatorInstruction},
types::Type,
value::ValueId,
};

Expand Down Expand Up @@ -68,9 +69,13 @@ fn value(function: &Function, id: ValueId) -> String {
}
Value::Function(id) => id.to_string(),
Value::Intrinsic(intrinsic) => intrinsic.to_string(),
Value::Array { array, .. } => {
Value::Array { array, typ } => {
let elements = vecmap(array, |element| value(function, *element));
format!("[{}]", elements.join(", "))
match typ {
Type::Array(_, _) => format!("[{}]", elements.join(", ")),
Type::Slice(_) => format!("&[{}]", elements.join(", ")),
_ => unreachable!("ICE: Value::Array with non-array type: {:?}", typ),
}
}
Value::Param { .. } | Value::Instruction { .. } | Value::ForeignFunction(_) => {
id.to_string()
Expand Down
14 changes: 10 additions & 4 deletions compiler/noirc_evaluator/src/ssa/ssa_gen/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -197,17 +197,23 @@ impl<'a> FunctionContext<'a> {
ast::Type::Array(_, _) => {
self.codegen_array_checked(elements, typ[0].clone())?
}
_ => unreachable!("ICE: unexpected array literal type, got {}", array.typ),
})
}
ast::Literal::Slice(array) => {
let elements =
try_vecmap(&array.contents, |element| self.codegen_expression(element))?;

let typ = Self::convert_type(&array.typ).flatten();
Ok(match array.typ {
ast::Type::Slice(_) => {
let slice_length =
self.builder.length_constant(array.contents.len() as u128);
let slice_contents =
self.codegen_array_checked(elements, typ[1].clone())?;
Tree::Branch(vec![slice_length.into(), slice_contents])
}
_ => unreachable!(
"ICE: array literal type must be an array or a slice, but got {}",
array.typ
),
_ => unreachable!("ICE: unexpected slice literal type, got {}", array.typ),
})
}
ast::Literal::Integer(value, typ, location) => {
Expand Down
19 changes: 19 additions & 0 deletions compiler/noirc_frontend/src/ast/expression.rs
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,17 @@ impl ExpressionKind {
}))
}

pub fn slice(contents: Vec<Expression>) -> ExpressionKind {
ExpressionKind::Literal(Literal::Slice(ArrayLiteral::Standard(contents)))
}

pub fn repeated_slice(repeated_element: Expression, length: Expression) -> ExpressionKind {
ExpressionKind::Literal(Literal::Slice(ArrayLiteral::Repeated {
repeated_element: Box::new(repeated_element),
length: Box::new(length),
}))
}

pub fn integer(contents: FieldElement) -> ExpressionKind {
ExpressionKind::Literal(Literal::Integer(contents, false))
}
Expand Down Expand Up @@ -319,6 +330,7 @@ impl UnaryOp {
#[derive(Debug, PartialEq, Eq, Clone)]
pub enum Literal {
Array(ArrayLiteral),
Slice(ArrayLiteral),
Bool(bool),
Integer(FieldElement, /*sign*/ bool), // false for positive integer and true for negative
Str(String),
Expand Down Expand Up @@ -498,6 +510,13 @@ impl Display for Literal {
Literal::Array(ArrayLiteral::Repeated { repeated_element, length }) => {
write!(f, "[{repeated_element}; {length}]")
}
Literal::Slice(ArrayLiteral::Standard(elements)) => {
let contents = vecmap(elements, ToString::to_string);
write!(f, "&[{}]", contents.join(", "))
}
Literal::Slice(ArrayLiteral::Repeated { repeated_element, length }) => {
write!(f, "&[{repeated_element}; {length}]")
}
Literal::Bool(boolean) => write!(f, "{}", if *boolean { "true" } else { "false" }),
Literal::Integer(integer, sign) => {
if *sign {
Expand Down
Loading
Loading