-
Notifications
You must be signed in to change notification settings - Fork 16
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
rules/minion: apply weighted sum more often (negatives, equality) #584
Conversation
9bd5c93
to
bb65323
Compare
I don't mind depending on identical-CSE in general. To understand the issue better here: so in your first example |
yes |
Looking again at abs-03, I think i'm missing a case here so will mark this as draft. |
I have one SumLeq / SumGeq left in the output, but I think it's the best I can do as the sum is inside an abs. May have missed a trick though! |
Savile row with -O0 also gets a SumLeq / SumGeq
Conjure Oxide:
|
Alternatively could you choose never to flatten inside |
I think we need it to do things like: ( a + b + c/d) * d = e Without flattening inside the sum: ( a + b + c/d) * d = e Then __1 would never get flattened. |
Originally I wanted to keep flattening in one place where possible. I see that the other way potentially could reduce auxvar usage though... |
In fact i was going to suggest merging flatten_unary, flatten_binary and flatten_vector as well, so I am with you on that. |
bc88505
to
eaf65ad
Compare
How about an alternative where we have a rule that matches a potential weighted sum expression, and does this style of auxiliary extraction in one go? I think you mentioned this as an option. Main thing I don't like with the PR as it stands is the cycle breaking. If we had a high-priority rule that handles the weighted sums that would fit in well with the rest of the bag-of-rules. |
I don't quite get what you mean - do you mean adding Eq handling inside the weighted sum rule? I think going from sum eq to constraints in one step instead of via inequalities might be better for both weighted and normal sums actually - it should reduce the number of aux variables needed. |
not sure what you mean with "Eq handling" I think the special case is that you don't want to extract an aux for |
Currently we do
We could remove
|
@ozgurakgun I have done the above - see the amended commit message for details. |
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.
the rest of the PR looks good, see comment.
These changes were motivated by the `basic/abs/03-flatten` test: it should be a weighted sum, but is not. With this commit, this becomes a weighted sum. * Remove sumleq / sumgeq rules, and merge these into the weighted sum rule. The weighted sum rule returns not applicable near the end if a normal sum should be used. Instead of this, this commit builds the normal sum here and returns it. * Add more cases to weighted sum, allowing negated expressions to be put inside the weighted sum. + -x ~> (-1,x) where x is an atom + -e ~> (-1,__1), with new constraint` __1=aux e` An alternative approach would be to add a normalisation rule `-e ~> -1 * e`; however, I don't think this should only happen inside sums, not in general. * Turn non-flat weighted sum equals expressions into weighted sums. * Remove `sum_eq_to_inequalities` and handle `x + y = z` and `2*x + 3*y =z` cases directly inside `introduce_weightedsumleq_sumgeq`. Expressions of the form `c*e1 + d*e2 + .. = t`, (where e1,e2 are non-flat) were not being turned into weighted sums. `sum_eq_to_inequalities` needs to run before a sumeq can be turned into a weighted sum, as `introduce_weighted_sumleq_sumgeq` only works on inequalities. However, flattening has a higher priority than this rule, which makes `introduce_weighted_sumleq_sumgeq` unapplicable to these expressions. (see 2833c5f (Convert weighted sums to Minion, 2025-01-07)). Furthermore, the priority change above causes an infinite cycle to occur between `flatten_binop` and `sum_eq_to_inequalities`: ``` sum([|a|, |b|]) = c ~~> sum_eq_to_inequalities (sum([|a|, |b|]) <= c /\ sum([|a|, |b|]) <= c) -- sum([|a|, |b|]) <= c ~~> flatten_binop __1 <= c with new top level constraints: __1 =aux sum([|a| , |b|]) -- sum([|a|, |b|]) =aux __1 ~~> sum_eq_to_inequalities (sum([|a|, |b|]) <= __1 /\ sum([|a|, |b|]) <= __1) ``` Both of these issues are fixed in this commit by removing `sum_eq_to_inequalities` and handling sum equals inside this rule.
c63367a
to
97bb67b
Compare
MacOS CI failure due to #595 |
Based on PR #583
Chain of upstream PRs as of 2025-01-10
PR Add absolute values and convert to Minion #583:
main
←nik/pr/add-abs/01
nik/pr/add-abs/01
←nik/pr/weighted-sum-neg-var
rules/minion: apply weighted sum more often (negatives, equality)
These changes were motivated by the
basic/abs/03-flatten
test: itshould be a weighted sum, but is not. With this commit, this becomes a
weighted sum.
Add more cases to weighted sum, allowing negated expressions to be put
inside the weighted sum.
__1=aux e
An alternative approach would be to add a normalisation rule
-e ~> -1 * e
;however, I don't think this should only happen inside sums, not
in general.
Turn non-flat weighted sum equals expressions into weighted sums.
Expressions of the form
c*e1 + d*e2 + .. = t
, (where e1,e2 arenon-flat) were not being turned into weighted sums.
sum_eq_to_inequalities
needs to run before a sumeq can be turnedinto a weighted sum, as
introduce_weighted_sumleq_sumgeq
only workson inequalites. However, flattening has a higher priority than this
rule, which makes
introduce_weighted_sumleq_sumgeq
unapplicable to theseexpressions. (see 2833c5f (Convert weighted sums to Minion, 2025-01-07)).
To fix this, this commit makes
sum_eq_to_inequalities
a higherpriority than the flattening rules.
A consequence of this is that more auxiliary variables are introduced
than before, because non-flat expressions are being duplicated.
For example:
I don't think this is a problem, however: because these are
syntactically identical, CSE should easily merge them (once we
implement it).
Do not flatten sums inside leqs/geqs
The priority change above causes an infinite cycle to occur between
flatten_binop
andsum_eq_to_inequalities
:To fix this, this commit adds a special case to
flatten_binop
toignore any Sum children of Leq and Geq.