Repository navigation
Conversation
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
23de899 to
6dbe5d9
Compare
j2kun
left a comment
j2kun
left a comment
There was a problem hiding this comment.
Looks great! Just some minor thoughts for your consideration.
| // | ||
| // Currently, non-determinism isn't hurting anything, so we finding a robust, | ||
| // cheap way to produce a globally-unique solution to the CPSAT instance as an | ||
| // open problem. But if you update the solver version, run this on a very big |
There was a problem hiding this comment.
A more robust approach (which is beyond the scope of this PR and maybe not worth the effort):
- Solve the model once, record the objective value OPT.
- Sort the decision variables deterministically (say, by walk order in the IR).
- For each decision variable
X, clone the model and addobjective <= OPT(as well as fixing decisions made in earlier iterations of this step); then add the constraintX = 0and add solution hints for all variables (that haven't been fixed yet) from the solution to (1). If the model is feasible, fixX=0going forward. Otherwise fixX=1. - Repeat step 3 until all variables are fixed.
I think with solver hints this would be fast enough, and at the end it gives the lexicographic minimal solution among all optimal solutions.
There was a problem hiding this comment.
I considered this option, but it seemed to expensive to me since it involved solving many CPSAT instances (all roughly the same size). I don't know what you mean by "solver hints", but the fact that we might be solving hundreds or thousands of instances seems prohibitive.
There was a problem hiding this comment.
You can pre-populate the solver with "hints" that it will use to seed initial attempts at finding feasible solutions. So if you populate this for all variables to known feasible value from an earlier solve, it should make the remaining solve very quick (though not guarnateed).
Anyhow, I just wanted to record this idea mainly for posterity.
| // solutions that minimize the primary objective, we pick one which | ||
| // (arbitrarily) minimizes the number of EVAL modes. `primaryScale` is a | ||
| // constant (i.e., independent of the mode assignments) which is always larger | ||
| // than `tieBreak`, therefore `objective * primaryScale + tieBreak` results in |
There was a problem hiding this comment.
I think the risk here is that scaling the objective up makes the solver risk hitting numerical instability issues. Though I think or-tools prints a warning if it detects that.
There was a problem hiding this comment.
I considered this, but I think 64 bits is sufficient for any realistic program. If there are 1 million mode variables (an absurd number, I think), that still leaves 40 bits of NTTs, which is also an absurd number.
| // this op is excluded from the single-poly-result restriction | ||
| // below: each of its successor inputs gets its own, independently |
There was a problem hiding this comment.
It seems like that with this PR there is no longer a need to care about how many polynomial-typed op results an op has ("ordinary" or otherwise). Maybe this restriction can be removed in a followup? If you agree we can file an issue for it.
There was a problem hiding this comment.
I don't think this PR changes anything here, the restriction is an artifact of how non-loop ops were handled initially. It could be removed, though I wouldn't make it a high priority as there aren't any ops that I'm aware of that return multiple polynomial results.
|
@crockeea please squash the commits and I'll get it in! |
This has the additional benefit of allowing function inputs and ouptuts to be optimized.
3687e3e to
98e4451
Compare
|
Squashed and pushed. |
j2kun
added
the
pull_ready
Fixes #2685, and replaces #3295.
This PR adds support for RegionBranchOpInterface to PolyMulToNTT, enabling NTT optimization inside things like for loops, if statements, and while loops.
Compared to #3295, this PR takes a more structured approach. It's more fiddly in the details, but the high-level approach is more obviously correct. It additionally allows the form of function inputs and outputs to be either COEFF or EVAL (whichever is optimal), while the existing code pins them to COEFF.
Specifically, #3295 fixes loop-carried values to COEFF form (if COEFF is needed) or EVAL otherwise, instead of allowing EVAL when both forms are needed and EVAL would be optimal. This is exactly what prevented NTTs from being lifted out of loops in #3295. In this PR, the form of all loop arguments is chosen by the solver (which also takes into account the number of times a loop runs), so lifting an NTT out of a loop (when optimal) is implicit in finding an optimal solution.
Assisted by AI, and contributed under individual license.