VSD - Cross-representation expression evaluation #5880
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
merging value_sets with value_sets
jezhiggins
requested review from
chrisr-diffblue,
martin-cs,
peterschrammel and
smowton
as code owners
The index_ranget has, by C++ standards, an unusual interface more akin to something you'd get on a C# enumerator. This is the first step to making it more container-like, with begin()/end() member functions returning iterators. With this commit, the iterators are input-iterator like, in that they can only be traversed once.
index_ranget is now only returns its begin and end iterators. It's now returned as a value, rather than through a shared_ptr, as its lifetime will naturally be scope limited and, as described below, polymorphic behaviour is hidden inside. This makes it more natural to use too, as we can just write for (const auto &index : range) rather than having to dereference it. It can now be repeatedly iterated, rather than the one and done it was in the previous commit. Behind it, the classes that do the actual work of enumerating the the different representations have been split out from input_ranget. There is no longer any need to share these objects, so they are instantiated using unique_ptr rather than shared_ptr.
Iterates over the values represented by the abstract object. For constants and intervals, this will be the object itself. For value sets, it will be the values in the set. Rework value_set expression evaluation in terms of value_ranget. This enables evaluation with mixed value_sets and constants. Made value_set_abstract_object implementation functions static. There's a bodge in value_set_abstract_objectt::unwrap_operands to workaround what would otherwise be an invariant violation. As we implement cross-representation evaluation, we'll hit these exceptions but once the work is complete they should all be eliminated.
Simple tests where all operands are constant_abstract_valuets, capturing and confirming existing behaviour.
Initial unit tests - currently fails
Implement abstract_value_objectt::expression_transform, and prevent further overriding. Within that, dispatch according to the types of the operands - * any are value sets, iterate over all of the combinations and dispatch for each of these, producing a value set to contain all of the results * mixed constant and interval, convert all constants to intervals and handle as all intervals * all interval, do interval arithmetic * all constant, substitute and simplify
The constants_evaluator class wraps up what was previous contained in constant_abstract_valuet::expression_transform. While it could have been implemented with free functions, using a class allows us to restrict scope, refactor to clearer code by reducing the number of parameters flying around. The process of building the class signposted a number of additional small clean ups that I might have otherwise overlooked.
Wrap up interval evaluation in a wrapper class. Most significant change is simplifying the process of coercing constants to intervals. Rather than trying to bail out early if we hit a top value or something which can't be converted, go over the whole list of operands, skipping any we can't convert. Check afterwards if we're missing some and then decide what to do.
for_each_comb doesn't need to be a template - it's only used once, so we know the type of the collection and we know the operation to be performed.
When we create values through the factory, the representation used is determined by the factory configuration. Up until now, that configuration has always lined up with the representation - eg if we were evaluating using intervals, then the factory is creating intervals. This is no longer the case. As we move forward with cross-representation evaluation, it's entirely possible that the factory configuration does _not_ match the representation we have in hand during an evaluation. Consequently, if we need a new constant value we should directly create it. As we leave the expression_transform, we use the factory to wrap any required context around the returned object.
Prevent repeated calls to obj->to_interval() by returning a list of expressions in the first place. Removes a level of indirection in the subsequent code.
jezhiggins
force-pushed
the
vsd-value-sets-and-constants
branch
from
79736ef to
2fa8d96
Compare
Codecov Report
@@ Coverage Diff @@
## develop #5880 +/- ##
===========================================
+ Coverage 72.90% 72.98% +0.07%
===========================================
Files 1423 1429 +6
Lines 154159 154687 +528
===========================================
+ Hits 112397 112899 +502
- Misses 41762 41788 +26
Continue to review full report at Codecov.
|
martin-cs
approved these changes
Mar 5, 2021
There was a problem hiding this comment.
First off, thank you for this large and impressive refactoring. I've been through each of the commits and they all seem to be doing the right kind of thing. Given the issues with the existing code and the scale of the restructuring I favour merging this and then I will do a review over all of VSD and pick up any issues that remain. I think that is the most efficient way forward.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Enable the variable-sensitivity domain to evaluate expressions involving different value representations.
For example, this enables evaluation of the expression
a + bwhere
ais a constant 3bis a value-set { 2, 4, 6 }returning a new value-set of { 5, 7, 9 }
All existing behaviour is retained - this is new additional behaviour.
Rather than each representation having its own
expression_transformeverything is now routed throughabstract_value_objectt::expression_transform, which then dispatches according to the operand representations. For all constants, and all interval expressions, the implementation is essentially identicalconstant_abstract_valuet::expression_transformandinterval_abstract_valuet::expression_transform. The value_set evaluation is broadly similar to what we had before, building each combination of operands. Each representation now has avalue_range, and the value_set evaluation uses this to build the combinations in a representation agnostic way.The focus of this PR has been on the interaction of value-sets and constants. There will be follow-up PR focussing on intervals - this may only be additional test-cases, but in any case will be rather less substantial.
The commit where the action happens is 8329afd Everything before that is essentially preparatory, everything after is refactoring enabled by this change.