-
Notifications
You must be signed in to change notification settings - Fork 123
Minimal range rules after head tail split #3511
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
base: main
Are you sure you want to change the base?
Conversation
…occurrence_variables
Co-authored-by: ShuangWu121 <[email protected]>
Co-authored-by: ShuangWu121 <[email protected]>
…labs/powdr into single_occurrence_variables
…o head-tail-split
| // 3. head is a linear expression coeff * var, | ||
| // then both head and tail must be zero. | ||
| // This rule replaces the variable var by zero. | ||
| struct ExpressionWithZeroMinimalRange(Expr); |
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.
We could call it NonNegativeExpression and instead of starting with ExpressionSumHeadTail, we could just use RangeConstraintOnExpression(e, rc) - it will work for all expressions. I also needed that in one of my PRs, so I think it will come in handy.
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.
Oh and we also need rc.range().1 < T::from(-1), otherwise it could also be unrestricted.
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.
Indeed. Done.
| struct MinimalRangeZeroDeducible<T: FieldElement>(Expr, Expr,Expr, Var, T); | ||
| MinimalRangeZeroDeducible(e, head, tail, var, coeff) <- | ||
| MinimalRangeAlgebraicConstraintCandidate(e), | ||
| ExpressionSumHeadTail(e, head, tail), |
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.
Aren't the following 4 lines already part of MinimalRangeAlgebraicConstraintCandidate?
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
chriseth
left a comment
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.
Looks good but I think this can still be simplified.
| // If an algebraic constraint e = 0 has the following properties: | ||
| // 1. expression e has range constraint [0, a] with a < P, | ||
| // 2. e = head + tail, and both head, tail >= 0, | ||
| // 3. head is a linear expression coeff * var, |
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.
Please correct me if I'm wrong, but I think we need as a vital component that rc(head) + rc(tail) is non-negative, i.e. does not wrap. I.e. it's not a property of the range constraint of e but rather of how we can combine the range constraints of head and tail.
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.
I am not sure I got it, since e = head + tail, isn't rc(head) + rc(tail) = rc(e)? This is an assumption though.
Do you see any case where
rc(e) < P - 1 holds, but rc_head.combine_sum(rc_tail).range().1 < P - 1 does not hold?
I can think about a case that rc(head) + rc(tail) wrapped but e is not:
rc(head) = [P-5, P-1]
rc(tail) = [10, 20]
rc(e) =[5, 19]
but the combine_sum function cannot distinguish this case, right?
In this minimal range zero rule, both head and tail has range start from zero, in this case, it seems to me:
rc(e) < P - 1 <=> rc_head.combine_sum(rc_tail).range().1 < P - 1
does this make sense?
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.
I think we have to be more specific with our notation here. The notation rc(head) is convenient but also problematic. Is it "just some range constraint that is compatible with the satisfying assignments to head" or is it "the currently best-known range constraint for head" or is it "the theoretically best range constraint for head"?
It can be that the best-known range constraint for e is "smaller" (i.e. a subset of) than the sum of the best-known range constraints for head and tail.
We could have a constraint X + Y = 0, then we have range constraints on X and Y that are 0..P-8 and then we have another range constrain on X + Y that say that it is either 0 or 1. Note that range constraints for expression can come from bus interactions. In this case, we cannot conclude that X = Y = 0, because another valid solution would be X = -10, Y = 10. I think we cannot work with range constraints for the expression (if it is an algebraic constraint) at all, because in the end, that expression is known to be 0 and only 0.
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.
I see. Thanks for the explanation!
I added the condition rc_head.combine_sum(rc_tail).range().1 < P - 1
now since rc_head.combine_sum(rc_tail).range().1 < P - 1 requires rc_head.range_width() + rc_tail.range_width() < P -1, the case (X + Y = 0, X, Y = [0..P-8], X = -10, Y = 10) should be ruled out, does this sound correct?
| (coeff != T::zero()); | ||
|
|
||
| //////////////////// RANGE PROPERTIES OF EXPRESSIONS ////////////////////// | ||
| struct NonNegativeExpression(Expr); |
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.
I think I suggested the name, but it's not strictly correct. But I also don't know a better name, so maybe it's enough to document the exact semantics?
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.
Added comment.
When you say “not strictly correct”, is that because this works over field elements, where “negative” isn’t really a well-defined concept?
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.
I mean this is also an aspect but usually, when we talk about signed numbers in the field, we say that 0.. P/2 are the non-negative numbers and the rest is the negative numbers. I.e. the range 0..(P-2) would also have negative numbers.
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.
I see. I documented its range, what about calling it NonWrappingExpression though?
| (e_rc.range().1 < T::from(-1)), | ||
| NonNegativeExpression(head), | ||
| NonNegativeExpression(tail), | ||
| AlgebraicConstraint(e); |
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.
Please check the performance, but I think putting AlgebraicConstraint(e) first should improve the speed.
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.
I tested this on keccak_small_prove_mock, and moving AlgebraicConstraint(e) first is indeed much faster.
Is there a general ordering principle for these facts that can improve performance?
| struct MinimalRangeAlgebraicConstraintCandidate(Expr); | ||
| MinimalRangeAlgebraicConstraintCandidate(e) <- | ||
| ExpressionSumHeadTail(e, head, tail), | ||
| RangeConstraintOnExpression(e, e_rc), |
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.
Please correct me if I'm wrong, but I think we need a statement that rc_head.combine_sum(rc_tail).range().1 < P - 1
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.
Done
Co-authored-by: chriseth <[email protected]>
| RangeConstraintOnExpression(tail, rc_tail), | ||
| (rc_head.range().0 == T::from(0)), | ||
| (rc_tail.range().0 == T::from(0)), | ||
| (rc_head.combine_sum(&rc_tail).range().1 < T::from(-1)); |
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.
If you use combine_sum here, then (following good practice and not assuming anything about the inner workings of combine_sum), it could still be that rc_head and rc_tail are unconstrained, even though all three of the conditions are met (since it could be -1..-2). Because of that, I think it would be better to use rc_head.range().1.to_arbitrary_integer() + ... - it should also be faster.
|
Does it reduce the overall timing? |
Co-authored-by: chriseth <[email protected]>
Co-authored-by: chriseth <[email protected]>
No, it actually make the performance worse. I measured the execution time of the optimizer, got
|

a + b = 0
a >= 0
b >= 0
then : a = 0, b = 0