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

Remove signed operation code from arithmetic table #812

Merged
merged 1 commit into from
Nov 11, 2022

Conversation

unzvfu
Copy link
Contributor

@unzvfu unzvfu commented Nov 10, 2022

We originally expected signed operations to be implemented in the arithmetic table, but as they are very very infrequently used, we have decided to implement them "in software" to save some arithmetic table columns. This PR removes the code and placeholders that were included prior to the decision to exclude these operations from the arithmetic table.

@unzvfu unzvfu self-assigned this Nov 10, 2022
@unzvfu unzvfu requested a review from typ3c4t November 10, 2022 00:28
Copy link
Contributor

@typ3c4t typ3c4t left a comment

Choose a reason for hiding this comment

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

though I'm unsure of the details that underlie the change from "in hardware" to "in software", this PR itself looks like a straightforward "config" change, and hence it LGTM---unsure if this causes any downstream effects, but if all checks pass...

@unzvfu
Copy link
Contributor Author

unzvfu commented Nov 11, 2022

though I'm unsure of the details that underlie the change from "in hardware" to "in software", this PR itself looks like a straightforward "config" change, and hence it LGTM---unsure if this causes any downstream effects, but if all checks pass...

Thanks Dmitry. I should have been clearer in the PR description, but there won't be any downstream effects; this is basically removing dead code.

@unzvfu unzvfu merged commit 56e291c into main Nov 11, 2022
@unzvfu unzvfu deleted the hamish/remove-signed-ops-from-arithmetic-table branch November 11, 2022 04:10
bhgomes added a commit to openzklib/plonky2 that referenced this pull request Nov 15, 2022
* Use static `KERNEL` in tests

* Print opcode count

* Update criterion

* Combine all syscalls into one flag (0xPolygonZero#802)

* Combine all syscalls into one flag

* Minor: typo

* Daniel PR comments

* Check that `le_sum` won't overflow

* security notes

* Test reverse_index_bits

Thanks to Least Authority for this

* clippy

* EVM shift left/right operations (0xPolygonZero#801)

* First parts of shift implementation.

* Disable range check errors.

* Tidy up ASM.

* Update comments; fix some .sum() expressions.

* First full draft of shift left/right.

* Missed a +1.

* Clippy.

* Address Jacqui's comments.

* Add comment.

* Fix missing filter.

* Address second round of comments from Jacqui.

* Remove signed operation placeholders from arithmetic table. (0xPolygonZero#812)

Co-authored-by: wborgeaud <williamborgeaud@gmail.com>
Co-authored-by: Daniel Lubarov <daniel@lubarov.com>
Co-authored-by: Jacqueline Nabaglo <jakub@mirprotocol.org>
Co-authored-by: Hamish Ivey-Law <426294+unzvfu@users.noreply.github.com>
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.

2 participants