-
Notifications
You must be signed in to change notification settings - Fork 280
Introduce expression enumerators #7080
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
Conversation
9a8b1bc
to
12b7ad8
Compare
Codecov ReportBase: 77.87% // Head: 77.99% // Increases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## develop #7080 +/- ##
===========================================
+ Coverage 77.87% 77.99% +0.12%
===========================================
Files 1574 1619 +45
Lines 181163 187185 +6022
===========================================
+ Hits 141072 146000 +4928
- Misses 40091 41185 +1094
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
12b7ad8
to
114b145
Compare
@SaswatPadhi, can you provide a first review of this so that we are making good use of the time of the DiffBlue reviewers. |
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.
I admit to not having read the entire PR, perhaps only the first half of it. As indicated in some comment, I would really appreciate some comments that explain the high-level idea. Various comments of mine hint at cleanup, which would make the code a lot more concise and thereby easier to read.
void set_non_exchangable() | ||
{ | ||
is_exchangable = false; | ||
} |
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.
What is this method about?
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.
Added a comment to explain exchangeable.
bool is_commutative(const irep_idt op) | ||
{ | ||
return op_id == ID_equal || op_id == ID_plus; | ||
} |
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.
What about multiplication, not-equal, and, or, bitand, bitor, bitxor? You might want to look at, or perhaps even call the functions from src/util/simplify_utils.cpp.
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.
Thanks for the reminder. simplify_expr() is now used to rule out more expressions. However, the optimization introduced in this class is still needed to avoid to enumerate redundant expressions.
Maybe I am getting mixed up but I remember @polgreen having some SyGuS / expression synthesis code built with CPROVER. Maybe with was @pkesseli ? Am hoping that @peterschrammel remembers... |
We did, but it wasn't enumerating: it was a symbolic encoding using selector variables in a fixed grammar and asking a decision procedure to find assignments to those selector variables that made the expression satisfy some spec. |
Thanks for the super-swift response @polgreen |
114b145
to
210246b
Compare
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.
Sorry, didn't spend a lot of time reviewing - please ping me when you want to re-review it.
ea47a16
to
fd2bac4
Compare
3530345
to
822fcbf
Compare
Thanks for the comments. I addressed most of the comments and added more explanation and examples. |
f93db8f
to
393c5e4
Compare
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.
A few more comments. This time I have managed to read all the code, and what I find the hardest to comprehend is the business of setting the size (is that the arity?). Why is the current approach the right one?
} | ||
|
||
std::list<partitiont> | ||
non_leaf_enumeratort::get_partitions(std::size_t n, std::size_t k) |
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.
So as to avoid having to convince myself that your algorithm is correct: how about using https://graphics.stanford.edu/~seander/bithacks.html#NextBitPermutation instead?
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.
Seconded. This took quite a bit of time to understand.
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.
Yes, I added an implementation that use the bithacks to enumerate bit permutation. However, such method only works for n <= 64 due to the length of size_t. I rewrote the old implementation using a similar idea of next-bit-permutation to handle n > 64
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.
You could use mp_integer
, but is it actually realistic to ever want n > 64? Depending on the value of k
this might get very expensive...
} | ||
|
||
std::list<partitiont> | ||
non_leaf_enumeratort::get_partitions(std::size_t n, std::size_t k) |
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.
Seconded. This took quite a bit of time to understand.
579c55a
to
6cd5d8e
Compare
I pushed an update including the following changes:
|
13571cb
to
a75e2d6
Compare
a75e2d6
to
f8e6230
Compare
This PR include expression enumerator interface that allow us to specify regular tree grammars over
exprt
and enumerate expressions from the grammar. It will be used in the enumerative loop-invariant synthesizer.We use a placeholder class to represent non-terminal symbols, enumerator classes to represent productions, and a factory class to represent grammars. A factory contain a map from placeholders to enumerators (non-terminal symbols to their productions). To enumerate expressions in the grammar with a given size, we repeatably expand placeholders with their productions until the expected size is reached. And then all productions without placeholders are instantiated to
exprt
objects.enumerator_baset
: base class of enumerators.leaf_enumeratort
: enumerators that enumerate leaf expressions---completeexprt
without placeholder.non_leaf_enumeratort
: enumerators that enumerate tree expressions of formop(a_1, ..., a_n)
whereop
is its operator symbol,n
is the arity ofop
, anda_i
are expressions enumerated by thei
-th sub-enumerator.binary_functional_enumeratort : non_leaf_enumeratort
: enumerators that enumerate trees with two children. It is optimized for boolean operators, comparison operators, and the operator+
.alternatives_enumeratort
: enumerators that enumerate expressions from the union of expressions enumerated by their sub-enumerators. For example, an enumerator for productions-> S + 1 | 1
in the grammarS -> S + 1 | 1
should enumerate the union of-> S + 1
and-> 1
.recursive_enumerator_placeholdert
: placeholders that represent non-terminals in the grammar.enumerator_factory_baset
: factories that maintain enumerators, placeholders, and the map from placeholders to enumerators.