Add bitwise air constraints with currently supported syntax #58
Conversation
tohrnii
changed the title
There was a problem hiding this comment.
Thanks @tohrnii this is a good addition to the tests. I left a few comments inline. Also, I think maybe we should remove this from the examples now that we've created a new test for it. I'm not sure that it makes sense to have the duplication or to have unsupported syntax in our examples (although we can use a variation of that file for the next version)
tohrnii
force-pushed
the
bitwise
branch
2 times, most recently
from
1217d1c to
edf5d84
Compare
|
Thank you @grjte for the review. Made suggested changes and fixes.
I've removed the examples directory entirely as we have all those files in the integration tests now. |
There was a problem hiding this comment.
Thanks @tohrnii, all the bitwise work looks good now. I don't think we should delete all the examples though, since in the docs & CLI examples we mention that they can be used for testing transpilation with the CLI. Maybe it would be best to just restore them for this PR and then submit a separate PR that has just one example which could be the example from the docs that includes all the syntax. What do you think?
Makes sense. I've opened #74 based on your suggestion. |
| let next = frame.next(); | ||
| result[0] = (current[0]).exp(E::PositiveInteger::from(2_u64)) - (current[0]) - (E::from(0_u64)); | ||
| result[1] = (periodic_values[1]) * (next[0] - (current[0])) - (E::from(0_u64)); | ||
| result[2] = (current[3]).exp(E::PositiveInteger::from(2_u64)) - (current[3]) - (E::from(0_u64)); |
There was a problem hiding this comment.
Comment for the future: exponentiations could be really expensive, especially for constant-time implementations (like the one we are currently using). So, whenever possible, we should replace them with multiplications or more specialized operations.
For example, here, instead of doing (current[0]).exp(E::PositiveInteger::from(2_u64)) we should be doing (current[0] * current[0]) or current[0].square().
Also, if we know that a constant value is smaller than u32 we should try to use conversions from u32. For example: E::from(2_u32) instead of E::from(2_u64) as we may come up with a more efficient reduction for smaller values later on.
This PR adds a modified version of existing bitwise air constraints to work with currently supported syntax and adds an integration test to check for expected code generation.