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(air): migrate range checker bus to LogUp #1027

Merged
merged 5 commits into from
Aug 7, 2023
Merged

feat(air): migrate range checker bus to LogUp #1027

merged 5 commits into from
Aug 7, 2023

Conversation

grjte
Copy link
Contributor

@grjte grjte commented Jul 28, 2023

This PR addresses #982 and #299 (no longer needed), simplifying the range checker to require only 2 main trace columns and a single auxiliary trace column.

  • rewrite AIR constraints
  • rewrite trace generation of main range checker trace
  • rewrite auxiliary trace generation for range checker lookup bus
  • update tests

@grjte grjte force-pushed the grjte-rc-logup branch 3 times, most recently from 5674e5f to e49f1ec Compare July 29, 2023 00:15
@grjte
Copy link
Contributor Author

grjte commented Jul 29, 2023

Notes:

  • the docs need a really big update, so I think I should tackle it in a separate PR
  • Currently we pad the range checker at the beginning of the trace. I think we could now pad at the end with rows of u16::MAX of multiplicity 0, and this would simplify things. Wdyt? I left it as is for now, to reduce the amount of changes.

@grjte grjte marked this pull request as ready for review July 29, 2023 00:24
@grjte grjte requested a review from bobbinth July 29, 2023 00:24
Copy link
Contributor

@bobbinth bobbinth left a comment

Choose a reason for hiding this comment

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

Looks great! Thank you! I left a few small blocking comments inline - some of them can be left for future PRs.

Re moving padding to the end - yes, let's leave it for a future PR. We can also combine this with one other potential optimization idea i left inline.

processor/src/range/mod.rs Outdated Show resolved Hide resolved
processor/src/range/mod.rs Show resolved Hide resolved
processor/src/range/mod.rs Outdated Show resolved Hide resolved
air/src/constraints/mod.rs Outdated Show resolved Hide resolved
air/src/constraints/range.rs Outdated Show resolved Hide resolved
processor/src/range/aux_trace.rs Outdated Show resolved Hide resolved
@bobbinth
Copy link
Contributor

bobbinth commented Aug 7, 2023

Oh - one other thing: let's update the changelog to include this change.

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