Conversation
brandonzstride
left a comment
There was a problem hiding this comment.
My comments are getting higher and higher level. I responded to an existing comment on to_propositional that will be especially impactful, I believe. To save energy in expectation of somewhat drastic changes in the wake of that comment, my review of integer.ml and solve.ml was rather quick.
I can really see this coming along now. Good work in handling so many of the comments from last time!
| let rec contains_binop : type a k. _ Binop.t -> (a, k) t -> bool = | ||
| fun target -> function | ||
| | Binop (op, l, r) -> | ||
| Binop.poly_equal op target || contains_binop target l | ||
| || contains_binop target r | ||
| | _ -> false |
There was a problem hiding this comment.
The change only covers not, which was strictly an example. The point is that this is not comprehensive, for example, And is not explored.
| | Binop | ||
| ( ((Less_than_eq | Less_than | Equal) as binop), | ||
| Const_int c, | ||
| Binop (((Plus | Minus) as op), Key (I a), Key (I b)) ) -> ( | ||
| match op with | ||
| | Plus -> | ||
| (* c <= a + b ==> c - b <= a *) | ||
| Formula.binop binop | ||
| (Formula.binop Minus (Formula.const_int c) (int_symbol b)) | ||
| (int_symbol a) | ||
| | Minus -> | ||
| (* c <= a - b ==> c + b <= a *) | ||
| Formula.binop binop | ||
| (Formula.binop Plus (Formula.const_int c) (int_symbol b)) | ||
| (int_symbol a) | ||
| | _ -> failwith "unreachable") |
There was a problem hiding this comment.
This suggestion applies to many cases above where convenient. The current code is reconstructing formulas when not necessary.
| | Binop | |
| ( ((Less_than_eq | Less_than | Equal) as binop), | |
| Const_int c, | |
| Binop (((Plus | Minus) as op), Key (I a), Key (I b)) ) -> ( | |
| match op with | |
| | Plus -> | |
| (* c <= a + b ==> c - b <= a *) | |
| Formula.binop binop | |
| (Formula.binop Minus (Formula.const_int c) (int_symbol b)) | |
| (int_symbol a) | |
| | Minus -> | |
| (* c <= a - b ==> c + b <= a *) | |
| Formula.binop binop | |
| (Formula.binop Plus (Formula.const_int c) (int_symbol b)) | |
| (int_symbol a) | |
| | _ -> failwith "unreachable") | |
| | Binop | |
| ( ((Less_than_eq | Less_than | Equal) as binop), | |
| Const_int c, | |
| Binop (((Plus | Minus) as op), (Key _ as a), (Key _ as b)) ) -> ( | |
| match op with | |
| | Plus -> | |
| (* c <= a + b ==> c - b <= a *) | |
| Formula.binop binop | |
| (Formula.binop Minus (Formula.const_int c) b) | |
| a | |
| | Minus -> | |
| (* c <= a - b ==> c + b <= a *) | |
| Formula.binop binop | |
| (Formula.binop Plus (Formula.const_int c) b) | |
| a | |
| | _ -> failwith "unreachable") |
There was a problem hiding this comment.
So that was what I was trying to do initially as well, but for cases where I explicitly reconstruct, it's because I come across an error like this:
The value b has type ($a1, 'a) Formula.t
but an expression was expected of type (int, 'b) Formula.t
Type $a1 is not compatible with type int
Hint: $a1 is an existential type bound by the constructor Binop.I agree with your approach since that also reduces the number of total function calls that need to be made using my solver. How can we make this work?
| let to_propositional | ||
| ?(to_symbol : int -> (bool, 'k) Symbol.t = | ||
| fun uid -> uid |> Uid.of_int |> fun uid -> Symbol.B uid) | ||
| (formula : (bool, 'k) Formula.t) = |
There was a problem hiding this comment.
The output is a pure SAT boolean formula whose symbols are associated with SMT formulas, then you should have a pure SAT boolean formula type to represent this. That way there is no mixing. This also would mean we can take out Or from the "occurs" binary operations, as we discussed offline at one point. So please make a boolean formula type instead of reusing Formula.t.
The current assumptions you make do not hold. For example, it seems like if k0 is the way I write a key with uid 0, then the following formula has a conflict. (k0) ^ (k1 = k2). The first formula falls into the | expr -> expr case and is untouched, and the second formula can be called k0 via to_symbol. If you argue that this Char.code 'p' thing changes anything, then just consider if k0 was replaced by whatever to_symbol 0 returns. Hence to_propositional (k0 ^ (k1 = k2)) returns (k0 ^ k0), which is bad.
| let rec is_idl_solvable : type k. (bool, k) Formula.t -> bool = | ||
| fun formula -> | ||
| match formula with | ||
| | Formula.And clauses -> | ||
| List.for_all is_idl_solvable clauses | ||
| | clause -> | ||
| is_idl_clause clause |
There was a problem hiding this comment.
This isn't especially important, but it is worth noting that this level of polymorphism on the key type k is not needed. The type k. syntax is necessary for polymorphic recursion, which means the type that k takes on may change within recursive calls. Here though, k just needs to be a standard polymorphic type variable. A locally abstract type (with (type k)) suffices to enforce the polymorphism necessary.
It may be helpful to some readers (including myself) to use exactly the required amount of polymorphic annotations.
| let rec is_idl_solvable : type k. (bool, k) Formula.t -> bool = | |
| fun formula -> | |
| match formula with | |
| | Formula.And clauses -> | |
| List.for_all is_idl_solvable clauses | |
| | clause -> | |
| is_idl_clause clause | |
| let rec is_idl_solvable (type k) (formula : (bool, k) Formula.t) : bool = | |
| match formula with | |
| | Formula.And clauses -> List.for_all is_idl_solvable clauses | |
| | clause -> is_idl_clause clause |
P.S. a simple 'k here (without the full (type k)) may even be best, but because it doesn't actually enforce any polymorphism, it technically is not the minimally required annotation. Some would say (type k) should be reserved for GADTs, and 'k. (bool, 'k) Formula.t -> bool here is the most appropriate if you annotate types at all.
There was a problem hiding this comment.
Ended up having to use the : type k ... = fun syntax
There was a problem hiding this comment.
Huh. Yeah I think that relates a certain OCaml issue that I don't want to link for sake of avoiding noise. But this works:
let rec is_idl_solvable (e : (bool, 'k) Formula.t) : bool =
match e with
| Formula.And clauses -> List.for_all is_idl_solvable clauses
| clause -> is_idl_clause clauseSorry for the originally wrong suggestion.
| let to_string | ||
| (type a k) | ||
| (model : k t) | ||
| ~(uid : Uid.t -> (a, k) Symbol.t * string) | ||
| : string = |
There was a problem hiding this comment.
This function can only print either the int or bool mappings from the model. Did you mean to have a polymorphic uid function? Like this?
let to_string
(model : 'k t)
~(uid : 'a. Uid.t -> ('a, 'k) Symbol.t * string)
: string =Alternatively, you may just make an assumption that the uid is packed into a key as-is, quite like is done in Grammar.Input_env.of_model. I think that makes usage easiest.
There was a problem hiding this comment.
I moved the packing logic from Grammar to be owned by Model:
type 'k key =
| Bool_key : (bool, 'k) Symbol.t -> 'k key
| Int_key : (int, 'k) Symbol.t -> 'k key
type 'k t =
{ value : 'a. ('a, 'k) Symbol.t -> 'a option
; domain : 'k key list }Now the to_string looks like this:
let to_string
(type k)
(model : k t)
~(key : k key -> string)
: string =
let indent = " " inLet me know if I did that wrong.
Features added
solve.mlinteger.mlboolean.mlOverview
I want to merge in my toy SMT solver ("Blue3") into the concolic evaluator so it can attempt to fast-solve certain formulas. The IDL solver and Boolean text parser are more-or-less ported over 1-to-1. The primary change from the port is writing out a basic DPLL (T) solver loop.
Benchmarks
Here are the benchmark results you can find by inputting
analysis.sqlinto an SQLite databaseafter running the benchmark script:
What were the average runtimes from both solvers?
Rows with the MIN times from both solvers
Rows with the MAX times from both solvers
How much faster were the fast cases on average?
How much slower were the slow cases on average?
What was the max time difference blue3 beat z3 by?
What was the max time difference z3 beat blue3 by?