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

Add binary search MASM example #160

Merged
merged 9 commits into from
Jan 11, 2024
Merged

Add binary search MASM example #160

merged 9 commits into from
Jan 11, 2024

Conversation

polydez
Copy link
Contributor

@polydez polydez commented Jan 7, 2024

This is a Miden Assembly example code, which performs binary search for a given value in a given ascending array. All input data should be provided in the operands_stack of intputs-file. If the operands_stack is empty, tests will run.

Copy link
Contributor

@Dominik1999 Dominik1999 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks Denis! This is a super cool MASM program. I left a couple of comments in line.

One point: We try to use the convention now that we first write what the next line is about and then indicate the final stack after the operations / line with # => [...]

like here

    # compute the pointer at which we should stop iterating
    dup.1 add
    # => [end_ptr, ptr, ...]

Can you change that quickly?
(I know it's not consistent yet with all the examples. I will change that.

We should combine this with the Bubble Sort of @phklive when merged.

#! Binary searches for value `val` in the memory addresses range [start..end), sorted in ascending order.
#!
#! Input: [start, end, val, ...]
#! Output: [found (true/false), addr] where `found` is searching result (`true`/`false`),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

where found is searching result (true/false)

nit: maybe better "where found indicates the result

#!
#! Input: [start, end, val, ...]
#! Output: [found (true/false), addr] where `found` is searching result (`true`/`false`),
#! `addr` is an address of value (if it was found), or an address where value should be inserted.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

addr is an address of value (if it was found), or an address where value should be inserted.

nit: can you add address in memory?

#! index `[0, count)` of `val`, if `val` was found; otherwise index in array, where `val` should be inserted.
proc.binary_search_stack
push.STARTING_MEMORY_ADDRESS # [STARTING_MEMORY_ADDRESS, count, arr[0], arr[1], ..., arr[count - 1], val, ...]
exec.read_stack_to_memory # [end, val, ...]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should it be write_stack_to_memort?

#!
#! Input: [addr, count, arr[0], arr[1], ..., arr[count - 1], ...]
#! Output: [end, ...] where `end` is addr next vacant memory address, thus array occupies addresses `[addr..end)` in the memory
proc.read_stack_to_memory
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same here, should it be write_stack_to_memory?

#! Output: [end, ...] where `end` is addr next vacant memory address, thus array occupies addresses `[addr..end)` in the memory
proc.read_stack_to_memory
u32assert2 # [addr, count, arr[0], arr[1], ...]
dup.1 # [count, addr, c, arr[0], arr[1], ...]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: [count, addr, count, arr[0], arr[1], ...]

if.true # [start, end, val, ...]
push.1 # [1, start, end, val, ...]
else # [start, end, val, ...]
# Push "not found" flag and flag to skip the loop
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would be an error, right?

You could use assert.err=123 and break.

Copy link
Contributor Author

@polydez polydez Jan 11, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not an error. Binary searching is used not only for searching, but also for adding new values into a sorted array. In a case when value wasn't found, the algorithm must return an index where this value should be inserted.

dup.1 # [end, start, end, val, ...]
dup.1 # [start, end, start, end, val, ...]
u32wrapping_sub # [count, start, end, val, ...], count = end - start
u32checked_div.2 # [count / 2, start, end, val, ...]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: can you add in the comment what happens if count is odd?

swap # [start, middle, end, val, ...]
drop # [middle, end, val, ...]
# Prepare "found" flag and exit the loop
push.1.0 # [0, 1, addr, end, val, ...], addr = middle
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the usage of the flags like this

dup # [count, count, ...]
neq.0 # [count != 0, count, ...]
if.true # [count, arr[0], arr[1], ..., arr[count - 1], val, ...]
exec.binary_search_stack # [found, index]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you maybe get rid of the found in the final output?

You could return all 0 if you didn't find the value and the pos of the stack if you found it

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think, it wouldn't be a good idea to return indexes of the stack. We work with index in array, which is passed through the stack. If value is not found, index == 0 would conflict with the index of 0th element. I tried to use special index like 0xFFFFFFFF for such cases, but it made the code worse.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

agreed, thanks for the explanation

swap # [found, index, ...]
end

proc.test_empty
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is super nice that you made tests!

Copy link
Contributor

@Dominik1999 Dominik1999 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me, thanks!

Copy link

Quality Gate Passed Quality Gate passed

Kudos, no new issues were introduced!

0 New issues
0 Security Hotspots
No data about Coverage
No data about Duplication

See analysis details on SonarCloud

@Dominik1999 Dominik1999 merged commit 877018b into 0xPolygonMiden:main Jan 11, 2024
@phklive
Copy link

phklive commented Jan 11, 2024

Congrats for your first PR!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants