Adding large-step encoding for CHCs in CProver (no [required] changes to the solver)#8465
Adding large-step encoding for CHCs in CProver (no [required] changes to the solver)#8465yvizel wants to merge 1 commit into
Conversation
yvizel
commented
Sep 22, 2024
yvizel
commented
- Each commit message has a non-empty body, explaining why the change was made.
- Methods or procedures I have added are documented, following the guidelines provided in CODING_STANDARD.md.
- The feature or user visible behaviour I have added or modified has been documented in the User Guide in doc/cprover-manual/
- Regression or unit tests are included, or existing tests cover the modified code (in this case I have detailed which ones those are in the commit message).
- My commit message includes data points confirming performance improvements (if claimed).
- My PR is restricted to a single feature or bugfix.
- White-space or formatting changes outside the feature-related changed lines are in commits of their own.
yvizel
requested review from
kroening,
peterschrammel and
tautschnig
as code owners
tautschnig
left a comment
tautschnig
left a comment
There was a problem hiding this comment.
A bunch of comments that each apply in several more places. More generally, however, it would be great to have comments. At bare minimum there should be high-level design comments in some place that explain how the various pieces fit together. Also, we need tests that demonstrate the new feature.
tautschnig
left a comment
tautschnig
left a comment
There was a problem hiding this comment.
clang-formatfixes are strictly required.- A bunch of suggestions below. Would be great to see them addressed, but we can do this in parallel with reviewing/working on the solver changes.
| @@ -0,0 +1,9 @@ | |||
| KNOWNBUG | |||
There was a problem hiding this comment.
Applies to this one as well as all other KNOWNBUG tests that you added: could you please add at the bottom:
--
Brief explanation as to what the bug is.
That is, add another -- and then an explanatory text.
| forall_exprt m_chc; | ||
|
|
||
| public: | ||
| horn_clauset(forall_exprt f) : m_chc(f) {} |
There was a problem hiding this comment.
Qualify this with explicit
| @@ -277,6 +277,7 @@ int cprover_parse_optionst::main() | |||
|
|
|||
| solver_options.trace = cmdline.isset("trace"); | |||
| solver_options.verbose = cmdline.isset("verbose"); | |||
| solver_options.large_step = cmdline.isset("large-step"); | |||
There was a problem hiding this comment.
Please add a help entry (in cprover_parse_optionst::help at the bottom of this file).
| #include <iostream> | ||
| #include <fstream> |
There was a problem hiding this comment.
Are either of those really required?
| cutpoint_grapht::~cutpoint_grapht() { | ||
| m_edges.clear(); | ||
| m_cps.clear(); | ||
| m_insts.clear(); | ||
| } |
There was a problem hiding this comment.
Why is the explicit deconstruction required?
| container_encoding_targett container2; | ||
| std::vector<horn_clauset> all2; |
There was a problem hiding this comment.
What are those good for?
| if (!can_cast_expr<forall_exprt>(clause)) | ||
| { | ||
| throw incorrect_goto_program_exceptiont("Not forall"); | ||
| } | ||
| const forall_exprt& forall = to_forall_expr(clause); | ||
| db.add_clause(forall); |
There was a problem hiding this comment.
I'd rewrite this using expr_try_dynamic_cast instead of can_cast_expr.
|
Also, once done with the changes, please rebase/combine all commits into a single one. (Or perhaps one for the source changes and one for the tests.) |
There was a problem hiding this comment.
Copilot reviewed 300 out of 541 changed files in this pull request and generated no comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #8465 +/- ##
===========================================
- Coverage 80.41% 80.24% -0.17%
===========================================
Files 1703 1707 +4
Lines 188398 188797 +399
Branches 73 73
===========================================
Hits 151502 151502
- Misses 36896 37295 +399 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
yvizel
force-pushed
the
develop
branch
2 times, most recently
from
cd2b757 to
0d6cdac
Compare
Introduce a resolution-based large-step encoding for Constrained Horn Clauses (CHCs) in CProver. The encoding is opt-in via a new `--large-step` command-line flag and does not require any changes to the underlying solver. New components added under src/cprover/: - chc_db: data structures for storing and indexing CHCs - chc_large_step: the large-step transformation built on top of chc_db - chc_wto: weak topological ordering of CHC predicates - cutpoint_graph: cutpoint graph used to drive the large-step encoding Hooks the new option through cprover_parse_options and extends state_encoding to emit the transformed clauses when the flag is set. Adds regression tests under regression/cprover/large_step/ covering arrays, pointers, structs, loops, function calls, and trace generation.
