-
Notifications
You must be signed in to change notification settings - Fork 63
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
Heapster memblock prover improvements #1406
Conversation
…on before eliminating it and would then go right back to trying to eliminate that same permission again...
…lock permission; changed proveVarLLVMBlocks to focus first on eliminating memblock permissions on the left that overlap with but are not contained in memblock permissions on the right
…are trying to prove on the right
…n empty block perm on the right
…s a tagged union type where we already know from something on the left what the tag is, in which case we can avoid searching for proofs of all the disjuncts other than the one that matches that known tag
…ence shape sh;emptysh with the empty shape when necessary
…rover-improvements
@@ -5423,7 +5572,7 @@ proveVarLLVMBlocks' x ps psubst mb_bps_in mb_ps = case mbMatch mb_bps_in of | |||
-- permission on the left with this exact offset, length, and shape, because | |||
-- it would have matched some previous case, so try to eliminate a memblock | |||
-- and recurse | |||
[nuMP| mb_bp : mb_bps |] | |||
[nuMP| mb_bp : _ |] | |||
| [nuMP| PExpr_NamedShape _ _ nmsh _ |] <- mbMatch $ fmap llvmBlockShape mb_bp |
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.
It's not a new change, but while we're looking at this function, it would be nice if we weren't duplicating mbMatch $ fmap llvmBlockShape mb_bp
so much. Each of these could be redoing the work of converting to pair representation.
This whole function could potentially benefit from doing a single [nuMP| mb_bp : mb_bps |]
pattern and handling the cases under that.
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.
Well, I started to look at how to implement this change, and it led to a fair bit of reorganizing proveVarLLVMBlocks
. The main changes are that it is now organized into two separate stages, represented by two separate functions, where the second stage / function takes a MatchedMb
for the shape as an explicit argument.
Another change is that these functions now take lists of multi-bindings instead of multi-bindings of lists. The justification is that those lists are always going to be converted to pair form and matched against anyway, so we might as well do that to begin with. Plus, there are a number of recursive calls that cons or prepend to those lists, and before they had to do this with fmap
, which converts back to function form and then again to pair form, while now this conversion has been removed. Because one of the main concerns with this change is performance, I did in fact time running Heapster on the rust_data
example before and after the change, and it seems to improve performance slightly, on the order of .1 - .2 seconds out of 17 seconds for the entire test. So I think that's a win?
da20fb7
to
d03220b
Compare
…y two separate functions, all of which now take lists of multi-bindings instead of multi-bindings of lists
Added a number of improvements to how the implication prover handles
memblock
permissions. Specifically:Changed
implElimLLVMBlock
to only perform one step of elimination formemblock
permissionsAdded more comments to
implElimLLVMBlock
Added
implElimAppendIthLLVMBlock
, which callsimplElimLLVMBlock
and then appends its results to the conjunctive permission on top of the stackAdded implIntroOrShapeMultiM to allow multiple nested or shapes to be all proved at once
Changed proveVarLLVMBlocks to use the new
implElimAppendIthLLVMBlock
when it eliminates memblock permissions to allow it to call itself again rather than callingimplElimPopIthLLVMBlock
and then going all the way to the top of proveVarImplAdded the notion of a tagged union shape, which is a multi-way nested or shape of the form sh1 orsh ... orsh shn where each shi starts with an equality permission to a concrete bitvector value that represents a tag for that disjunct. Also added implication prover for recognizing when we are trying to prove a tagged union shape and already know which tag to choose, in which case we avoid doing the search across all n disjuncts and instead choose the right one from the beginning.
Added debug output for type-checking return statements