abby: always store nextgen region constraints in canonical form - #161306
Conversation
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
d860264 to
c64bf00
Compare
This comment has been minimized.
This comment has been minimized.
c64bf00 to
57b2d5e
Compare
This comment has been minimized.
This comment has been minimized.
57b2d5e to
c17800a
Compare
This comment has been minimized.
This comment has been minimized.
aba4012 to
56320f9
Compare
This comment has been minimized.
This comment has been minimized.
5bedfe3 to
65f3b73
Compare
This comment has been minimized.
This comment has been minimized.
a10cd16 to
930ca67
Compare
This comment has been minimized.
This comment has been minimized.
930ca67 to
1bead00
Compare
This comment has been minimized.
This comment has been minimized.
989777a to
4eebcea
Compare
4eebcea to
b22bffa
Compare
|
This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed. Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers. |
|
@bors r+ rollup=never |
|
@bors r=lcnr,khyperia |
This comment has been minimized.
This comment has been minimized.
What is this?This is an experimental post-merge analysis report that shows differences in test outcomes between the merged PR and its parent PR.Comparing 4fcf397 (parent) -> d8df826 (this PR) Test differencesShow 16 test diffsStage 0
Stage 1
Additionally, 10 doctest diffs were found. These are ignored, as they are noisy. Job group index
Test dashboardRun cargo run --manifest-path src/ci/citool/Cargo.toml -- \
test-dashboard d8df82673d5911b6112a85bf91d9adefb2c66a1a --output-dir test-dashboardAnd then open Job duration changes
How to interpret the job duration changes?Job durations can vary a lot, based on the actual runner instance |
|
Finished benchmarking commit (d8df826): comparison URL. Overall result: ❌ regressions - please read:Our benchmarks found a performance regression caused by this PR. Next Steps:
@rustbot label: +perf-regression Instruction countOur most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.
Max RSS (memory usage)Results (primary -1.7%, secondary -1.9%)A less reliable metric. May be of interest, but not used to determine the overall result above.
CyclesResults (secondary 6.8%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Binary sizeResults (primary -0.0%, secondary -0.1%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Bootstrap: 476.065s -> 477.112s (0.22%) |
View all comments
title. introduce an
And/Or/LeafConstraint/ types to reason about the structure of our region constraints. Never produce arbitrarily nested or/ands and always have constraints in an evaluated form.I kinda mucked up this PR and accidentally did two things at the same time. Not only do we immediately put everything into canonical form, we also change what it means for a region constraint to be in canonical form. Whoops :>
Rough overview of what a
RegionConstraintis:RegionConstraintcontains two things: anAND of LEAFsand anOR of AND of LEAFs. Another way of thinking about it would be to say its anANDconsisting of arbitrarily many LEAFs and a singleOR of AND of LEAFsfalsethen we wipe the top levelANDas it doesn't matter what they are, the constraint is always going to be falsethis simplifies a lot of things conceptually as we now no longer need to worry about what state our region constraints are in. and our algorithms also don't need to handle arbitrary nesting of ors/ands :) and its a lot easier to read the debug logs 😅
I also wound up needing to do this while trying to compile
std/corewith-Zassumptions-on-bindersas we would otherwise OOM from having both:And('a: 'b, 'a: 'b))OR(e.g.Or(And('a: 'b, 'b: 'c), And('a: 'b, 'b: 'd)))Some future work:
RegionConstraint::splatted_and_constraintsit's kind of weird to even need it and probably encourages bad-for-perf patternsRegionConstraint. Perf stuff :3In theory this PR should mostly not have functional changes. In practice it might affect some things due to changing the exact repr of things affecting query responses. There's probably also some behaviour differences here due to us falling on our face more or less in WIP parts of abby now that we have different region constraints. I don't think any of this should be meaningful though. This PR is intended to not fundamentally change the abby algorithm :3
This PR should be reviewed commit-by-commit. There are a bunch of commits restructuring existing logic to assume their input is in canonical form as it will be by the end of the PR.
Then there's the core change in
always canonical formwhich actually replacesRegionConstraintwith all the new types and updates all the locations using them.Finally there's
propagate ambiguity not evaluatewhich deals with the leftoverevaluate_solver_constraintwhich was mostly unnecessary now due to moving its main logic into construction ofRegionConstraintand friends. I didn't want to make actual bug fixes in this PR so I just left some FIXMEs about some of the issues thatpropagate_ambiguityhas instead of fixing them here.Fixes rust-lang/project-assumptions-on-binders#14