chore(ssa refactor): Add code to handle array equality #1635
Conversation
…t brillig are no longer in brillig_gen
kevaundray
changed the base branch from
master
to
kw/refactor-brillig-abstraction-layer
| Instruction::ArrayGet { array, index } => { | ||
| let array_ptr = self.convert_ssa_value(*array, dfg); | ||
| let index = self.convert_ssa_value(*index, dfg); | ||
|
|
||
| let result_ids = dfg.instruction_results(instruction_id); | ||
| let destination = self.get_or_create_register(result_ids[0]); | ||
|
|
||
| self.context.array_get(array_ptr, index, destination) | ||
| } |
There was a problem hiding this comment.
Will remove this is not needed for array_eq example
| match operator { | ||
| BinaryOp::Eq => { | ||
| // TODO: | ||
| // Add a for loop which does roughly |
There was a problem hiding this comment.
Currently this code returns 1 for array equality; just need to implement the for-loop noted below
| // Note: We could insert a const instruction to initialize the register | ||
| // because the VM will not expand the register space automatically. | ||
| // | ||
| // In most cases, the register created is used in another instruction | ||
| // which will cause the VM to expand the register space, but this is not | ||
| // a guarantee. | ||
| // | ||
| // TODO: check if the above can be true if we just return a constant for example | ||
| // TODO from a program | ||
| // self.const_instruction(register, 0u128.into()); |
There was a problem hiding this comment.
Will remove, noting it here as it deserves a separate issue. It can only lead to bugs, if we create a register and then not store a value in it, or not store a value in a register whose index is larger than that register.
| BinaryOp::Eq => { | ||
| // Allocate one array for the equality comparisons | ||
| let comparisons_array_ptr = self.context.create_register(); | ||
| self.context.allocate_array(comparisons_array_ptr, num_elements, false); |
There was a problem hiding this comment.
Seems we are doing something akin to inlining the bytecode since the num_elements is known -- we probably want to do it the way that assembly does it which is by using a for-loop with labels. I can push an unoptimized version
| let allocation = self.memory.allocate(size as usize); | ||
|
|
||
| // If the array is prefilled (for example, parameter arrays), then we do not need to expand memory | ||
| if !prefilled { |
There was a problem hiding this comment.
I can see how this fixes the bug, though I wonder if this is leaking an abstraction -- ie parameter arrays are not a concept in this part of the code.
Will investigate the ssa ir!
| start | ||
| Allocation { | ||
| start_address: MemoryAddress(start), | ||
| end_address: MemoryAddress(self.free_mem_pointer - 1), |
| // The features being tested is using array equality on brillig | ||
| fn main(x: [Field;3], y : [Field;3]) { | ||
| assert(array_eq(x, y)); | ||
| assert(!array_eq(x, [1,2,4])); |
There was a problem hiding this comment.
We probably want to add the edge case here, for empty arrays -- it should return true in that case.
An issue to open is to check if the ssa ir will give a compiler error on arrays of different lengths. If it gets to brillig ir, then it will panic in the type_of_binary_operation function
There was a problem hiding this comment.
Empty arrays don't seem to be allowed
error: Arrays must have at least one element
And with different sizes the compiler issues
error: Can only compare arrays of the same length. Here LHS is of length 3, and RHS is 2
|
I believe this PR will no longer be necessary when #1704 is merged |
|
#1704 has been merged so closing |
Description
Problem*
Resolves
Summary*
This PR sets out to
Example
Before:
After:
Documentation
This PR requires documentation updates when merged.
Additional Context
PR Checklist*
cargo fmton default settings.