-
Notifications
You must be signed in to change notification settings - Fork 87
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
Improve RangeChip
with tagged table
#38
Improve RangeChip
with tagged table
#38
Conversation
46eb3f1
to
4505bf1
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
I just left some suggestions that we might want to adopt. Up to you :)
Feel free to address what you think it's fine to and merge!
Thanks for this great work! 🎉
maingate/src/range.rs
Outdated
let config = TableConfig { | ||
selector: meta.complex_selector(), | ||
column: meta.lookup_table_column(), | ||
let (s_overflow, tag_overflow) = if has_overflow_limb { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe? Just a suggestion to save the creation of a variable that might be avoidable. I say it more for ease of read than performance of course.
let (s_overflow, tag_overflow) = if has_overflow_limb { | |
let (s_overflow, tag_overflow) = if !overflow_bit_lens.is_empty() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated in 0ca396e
maingate/src/range.rs
Outdated
macro_rules! lookup { | ||
($prefix:literal, $selector:ident, $tag:ident, $bit_lens:ident, [$($value:ident),*]) => { | ||
$( | ||
meta.lookup(concat!($prefix, stringify!($value)), |meta| { | ||
let selector = meta.query_selector($selector); | ||
let tag = match $tag { | ||
Some(tag) => meta.query_fixed(tag, Rotation::cur()), | ||
None => { | ||
let tag = F::from(bit_len_tag[&$bit_lens[0]] as u64); | ||
selector.clone() * Expression::Constant(tag) | ||
}, | ||
}; | ||
let value = meta.query_advice($value, Rotation::cur()); | ||
vec![(tag, t_tag), (selector * value, t_value)] | ||
}); | ||
)* | ||
}; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generally I try to avoid macros. But this one in particular is clear, easy to understand and with a really tiny scope. So it's fine at least for me to keep it as is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Anyway, feels like it could be replaced by a function or closure.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated to function in 0ca396e.
transcript/src/transcript.rs
Outdated
|
||
/// `PointRepresentation` will encode point with an implemented strategy | ||
/// TODO: Generalize `ecc_chip` with `EccInstrucitons` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we file an issue for this and remove the TODO
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Already filed the issue. Feel free to just remove this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed in 0ca396e
Feel free to merge when all the checks pass @han0110 🎉 |
Use tagged table in
RangeChip
to reduce number of lookup.Resolves #37