-
Notifications
You must be signed in to change notification settings - Fork 76
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
Widening gas #1442
base: master
Are you sure you want to change the base?
Widening gas #1442
Conversation
Why does this need to be inside the solver? This sounds like widening delay, which can be completely solver-independent and can be implemented simply as a lifting of a domain (overriding its |
@sim642 One could implement a similar feature as a domain lifter, yes. However, the effect would be subtly different: Here the delay (gas) is applied per unknown, whereas tracking it in the domain would mean that the gas value is propagated, e.g., between different loops, or that a complicated mechanism tracking not just one gas value but several inside a map would be required. |
I think the standard implementation is just domain lifting where transfer functions set the counter to 0, so none of such propagation occurs. I'm again worried about the monolithic growth of the TD3 solver. For a long time we've wanted to undo that monolithic structure into a more modular one. While nobody has worked on decomposing the existing solver, at some point we decided to not worsen the problem and implement new things modularly. For example, |
This seems like a policy discussion, so I cannot really say much. |
I think it would even be easier: one can choose whether to wrap an analysis's |
We discussed this during GobCon and here's the summary. Currently, The change from Some small style improvements should also be done to make this merge-ready. It was mentioned that there are some parentheses which are unneeded (e.g. around |
I have extracted the modules out of td3.Base. For now, I have placed them in their own module in
So far, no other solver implements these strategies, so I have left the selection of the concrete implementation in td3. In their current form, I would not say that the strategies are really independent from td3. Some of the functions/strategies need td3-specific details, like an |
src/solver/td3.ml
Outdated
(* is there a reason for the redundant widen check? *) | ||
if tracing && not (S.Dom.is_bot old) && should_widen x then trace "solchange" "%a (wpx: %b): %a" S.Var.pretty_trace x (should_widen x) S.Dom.pretty_diff (wpd, old); |
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 comment 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.
My comment is strangely worded, but I think it is odd that should_widen
or previously HM.mem wpoint x
is re-evaluated here.
Specifically, let wp = should_widen x
before evaluating eq
is used to determine, whether widening should be applied. The result of should_widen
may have changed through eq
, yet I would assume the value that is logged should reflect the value that is used for the actual widening decision.
But if using the updated value, I should be logging with format_wpoint
, which will not only show whether x is a wpoint
, but also how much gas it has.
src/solver/td3.ml
Outdated
if term && phase = Widen && should_widen x then ( (* TODO: or use wp? *) | ||
if tracing then trace "sol2" "solve switching to narrow %a" S.Var.pretty_trace x; | ||
if tracing then trace "sol2" "stable remove %a" S.Var.pretty_trace x; | ||
HM.remove stable x; | ||
HM.remove superstable x; | ||
Hooks.stable_remove x; | ||
(solve[@tailcall]) ~reuse_eq:eqd x Narrow |
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.
Is it right to swap HM.mem wpoint
for should_widen
here?
This case is taken when in the widening phase the value has stabilized and thus should be switched to narrowing.
If I understand correctly, this now means that if all the widening has isn't used up at x
(because should_widen
requires it to be 0), then the solver doesn't switch to narrowing at all.
Previously the notions of "widening point" and "should apply widening now" more-or-less coincided, but with gas they no longer do. I think places like this that check HM.mem wpoint
but don't actually call widen
based on that should still check for "widening point", not "should apply widening now".
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 think it should be appropriate to use should_widen
here. If x is a widening point with gas, that means it was never widened. If it was not widened, then narrowing would be pointless.
On the other hand, using HM.mem wpoint
would also not be harmful either, nor would narrowing cause a large overhead if x does not change. I can change it to a membership check, if you prefer
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.
Ah, I guess you are right, narrowing could still have an effect due to some non-monotonicity.
let widen a b = | ||
if M.tracing then M.traceli "sol2" "side widen %a %a" S.Dom.pretty a S.Dom.pretty b; | ||
let r = S.Dom.widen a (S.Dom.join a b) in | ||
if M.tracing then M.traceu "sol2" "-> %a" S.Dom.pretty r; | ||
r | ||
in | ||
let old_sides = HM.find_default sides y VS.empty in | ||
let op a b = match side_widen with | ||
| "sides-local" when not (S.Dom.leq b a) -> ( |
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.
This leq
check is not present in SidesLocal.veto_widen
.
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.
That is true, but if b is less or equal to a, widening should have no effect, right?
It would've been fine to leave them in |
for globals unknowns: * can be used together with other widening strategies * reduce gas whenever global grows * widen only if marked as widening point by side_widen and gas is 0
b65cdc3
to
493732e
Compare
Description
This PR adds widening gas to the td3 solver. Widening gas permits an unknown to grow N times and still be combined with the regular join. Only once the unknown runs out of gas, the widening operator is used. Widening gas can be applied to regular unknowns and leaf unknowns.
Additionally, this PR extracts the side_widen strategies out of the main solve routine and wraps them in an interface.
Config Options
The gas budget for regular unknowns is controlled via
solvers.td3.widen_gas
, whereas leaves are controlled viasolvers.td3.side_widen_gas
. Both config options are 0 by default, effectively disabling the feature.When widening points are configured to be reset with the
solvers.td3.remove-wpoint
option, this too resets that wpoints gas.New side_widen Interface and Gas for Leaves
Widening gas co-exists with the other side-widening strategies.
Side widen strategies are meant to determine, when a leaf becomes a widening point. After a leaf has been selected as widening point, future updates to its value are applied with the widen operator. The widening gas can again be used to postpone this:
After a leaf has been marked as widening point, its gas counter is decreased for every increase to its value. Once it hits 0, widening is applied.
There is one exception, namely the side_widen strategy
sides-local
. It does not mark widening points, instead widening only if the incoming side-effect is larger than a previous side-effect that came from the same unknown. This has been adapted to work with widening gas like so: every time that thesides-local
strategy would widen, the gas is decremented.Functions of the new interface:
Note that
notify_side
is needed forunstable-self
, which records side-effects in theinfl
set.veto_widen
is necessary to implement the above behavior forsides-local
.record_destabilized_vs
is currently only used by thecycle
strategy. It indicates, whether the more expensive version ofdestabilize
needs to be called, which also records whether a called or start variable was destabilized.