Skip to content
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

ff: kinds file and deps (enumerator, type-checker, rewriter) #9059

Merged
merged 9 commits into from Sep 1, 2022

Conversation

alex-ozdemir
Copy link
Member

Add the ff kinds file and immediate dependencies.

The diff is huge, and I'd like to shrink it before proper review. Suggestions? I describe the contents of this PR below, as a dependency tree:

  • kinds file
    • Theory class: everything stubbed out (big)
      • new theory ID
    • type enumerator
    • type rules (medium)
    • theory rewriter (big)
      • small modifications to node/node manager

I guess I could stub out chunks of the type rules and theory rewriter? I guess I could also omit some parts of the kinds file, but I don't really know what parts depend on what, or if you are allowed to omit parts.

(somehow the generated file is still on my machine?)
@@ -311,6 +311,11 @@ std::string LogicInfo::getLogicString() const {
ss << "BV";
++seen;
}
if (d_theories[THEORY_FF])
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd just remove this (this is the reason why the PR is failing CI). For now, I would suggest enabling finite fields only when the logic is set to ALL.

Logic strings are in need of refactoring (at the SMT-LIB standard level), so this code will likely be rewritten later.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm. So most of my CLI regression tests (yet un-merged) use the QF_FF logic string, so I left this code and just fixed the failing test.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok no worries.

@@ -445,6 +450,11 @@ void LogicInfo::setLogicString(std::string logicString)
enableTheory(THEORY_BV);
p += 2;
}
if (!strncmp(p, "FF", 2))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Delete this as well.

Copy link
Member

@4tXJ7f 4tXJ7f left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some minor comments. Btw. looking at FiniteField again, it seems like the name could be a bit confusing. What do you think about renaming it to FfVal/FiniteFieldVal/FiniteFieldValue?

src/theory/ff/type_enumerator.h Outdated Show resolved Hide resolved
src/theory/ff/type_enumerator.h Outdated Show resolved Hide resolved
namespace theory {
namespace ff {

// static
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// static

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

src/theory/ff/theory_ff_rewriter.cpp Outdated Show resolved Hide resolved
src/theory/ff/theory_ff_rewriter.cpp Outdated Show resolved Hide resolved
src/theory/ff/theory_ff_type_rules.cpp Outdated Show resolved Hide resolved
src/theory/ff/kinds Outdated Show resolved Hide resolved
src/theory/ff/theory_ff.h Outdated Show resolved Hide resolved
src/theory/ff/theory_ff.h Outdated Show resolved Hide resolved
src/theory/ff/theory_ff.cpp Outdated Show resolved Hide resolved
alex-ozdemir and others added 2 commits August 25, 2022 22:09
Co-authored-by: Andres Noetzli <andres.noetzli@gmail.com>
@alex-ozdemir
Copy link
Member Author

Thanks for the review, Andres. I've address the specific comments, and re-named FiniteField to FfVal as suggested.

@ajreynol ajreynol self-requested a review August 30, 2022 16:19
Copy link
Member

@4tXJ7f 4tXJ7f left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks for the updates! Especially the renaming to FfVal :)

@4tXJ7f 4tXJ7f enabled auto-merge (squash) September 1, 2022 19:27
4tXJ7f added a commit to 4tXJ7f/cvc5 that referenced this pull request Sep 1, 2022
Previously, we had no configuration that tested the `--cocoa` option.
This lead to broken unit tests when the option was enabled. PR cvc5#9059
fixed one of the unit tests. This commit adds `--cocoa` to a
configuration to ensure that this does not happen in the future.
@4tXJ7f 4tXJ7f merged commit 0df9899 into cvc5:main Sep 1, 2022
@alex-ozdemir alex-ozdemir deleted the ff-boiler branch September 2, 2022 07:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
moderate Complexity normal Priority
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants