Skip to content
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

fixing issue #4651 #4666

Merged
merged 80 commits into from
Sep 8, 2020
Merged

fixing issue #4651 #4666

merged 80 commits into from
Sep 8, 2020

Conversation

veanes
Copy link
Collaborator

@veanes veanes commented Aug 27, 2020

Added rewrite rules to lift to_re from within ite-expressions.
Also disabled lift_ites_throttled that seems to cause performance issues (in this example 100x slowdown).
Updated also pp of regexes to indicate where a regex is not fixed.
The underlying issue of #4651 is fixed or at least does not show up with this example.

@@ -728,9 +728,11 @@ br_status seq_rewriter::mk_app_core(func_decl * f, unsigned num_args, expr * con
case _OP_STRING_STRIDOF:
UNREACHABLE();
}
#if 0
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this will create a perf regression for strings, where lifting ites is very useful.
So it could look at the type of the expression. If it is a seq, then lift, otherwise don't.
That can be accomplished inside lift_ites_throttled, not here.

@@ -3811,7 +3840,15 @@ br_status seq_rewriter::mk_re_star(expr* a, expr_ref& result) {
result = re().mk_star(re().mk_union(b1, c1));
return BR_REWRITE2;
}
if (m().is_ite(a, c, b1, c1))
{
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

move { to after ), not on a new line as in C#

else if (re.u.str.is_empty(s))
out << "()";
else
seq_unit(out, s);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this makes a "long distance" assumption about values.
Better to not make such assumptions deeply buried in code and have the routine more self-contained.
You can simply check for is_concat(s) regardless of whether it is a value, and you can check for unit, and empty as 3 separate cases. The is_value check only filters cases of concatenations of non-constants and constants, where the conventions in this routine are not uniform: in the default case you use (to_re ...), in the non-default case it doesn't use (to_re ..) around the expression.

So instead

if (is_empty(s))
    out << "();
else if is_unit(s))
      seq_unit(s, s);
else if (is_concat(s)) 
    for (expr* e : concats(s))
          concat_helper_sec(out, s);
else out << "(to_re " << pp(s) << ")";

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I updated it similarly, but essentially I wanted to distinguish between 'A' vs uninterpreted const A, the latter is now printed as {A}
Another thing I wanted to distinguish is (in some cases) pp(to_re S) from pp(S), but in most cases the distiction is not needed or desired and just adds noise, so I did not do it in the end. e.g. pp(tore("ab") ++ tore("cd") ) and
pp(tore("ab" ++ "cd")) are printed the same way

NikolajBjorner and others added 4 commits August 27, 2020 15:05
Signed-off-by: Nikolaj Bjorner <[email protected]>
Signed-off-by: Nikolaj Bjorner <[email protected]>
…filter finding the model in report Z3Prover#4651 goes from .02s to 20s, also updated pretty printing of regexes to be more accurate
{
// do not lift over regexes
// for example DO NOT lift to_re(ite(c, s, t)) to ite(c, to_re(s), to_re(t))
if (is_sort_of(f->get_range(), get_fid(), RE_SORT))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is too low level, we have higher level checks, or add them if they don't already exist:

  • u.is_re(f->get_range())
  • u.is_seq(f->get_range())
  • a.is_arith(m().get_sort(ite))

vlsergey and others added 20 commits August 28, 2020 07:30
Signed-off-by: Nikolaj Bjorner <[email protected]>
Signed-off-by: Nikolaj Bjorner <[email protected]>
Signed-off-by: Nikolaj Bjorner <[email protected]>
Signed-off-by: Nikolaj Bjorner <[email protected]>
Signed-off-by: Nikolaj Bjorner <[email protected]>
Signed-off-by: Nikolaj Bjorner <[email protected]>
Signed-off-by: Nikolaj Bjorner <[email protected]>
Signed-off-by: Nikolaj Bjorner <[email protected]>
Signed-off-by: Nikolaj Bjorner <[email protected]>
Signed-off-by: Nikolaj Bjorner <[email protected]>
Signed-off-by: Nikolaj Bjorner <[email protected]>
Signed-off-by: Nikolaj Bjorner <[email protected]>
Signed-off-by: Nikolaj Bjorner <[email protected]>
Signed-off-by: Nikolaj Bjorner <[email protected]>
Signed-off-by: Nikolaj Bjorner <[email protected]>
Signed-off-by: Nikolaj Bjorner <[email protected]>
Signed-off-by: Nikolaj Bjorner <[email protected]>
Signed-off-by: Nikolaj Bjorner <[email protected]>
NikolajBjorner and others added 24 commits September 1, 2020 10:19
Signed-off-by: Nikolaj Bjorner <[email protected]>
Signed-off-by: Nikolaj Bjorner <[email protected]>
Signed-off-by: Nikolaj Bjorner <[email protected]>
Signed-off-by: Nikolaj Bjorner <[email protected]>
Signed-off-by: Nikolaj Bjorner <[email protected]>
Signed-off-by: Nikolaj Bjorner <[email protected]>
Signed-off-by: Nikolaj Bjorner <[email protected]>
Signed-off-by: Nikolaj Bjorner <[email protected]>
Signed-off-by: Nikolaj Bjorner <[email protected]>
Signed-off-by: Nikolaj Bjorner <[email protected]>
Signed-off-by: Nikolaj Bjorner <[email protected]>
Signed-off-by: Nikolaj Bjorner <[email protected]>
…filter finding the model in report Z3Prover#4651 goes from .02s to 20s, also updated pretty printing of regexes to be more accurate
…inting with regex tooltip generated in dgml state graph
@veanes
Copy link
Collaborator Author

veanes commented Sep 5, 2020

While the branch is named fix4651, it only partially addresses use of nonground regexes in the related issue. This is work in progress. But the PR does fix one crucial bug where state ids were off by one in lookup table, luckily this lookup table was not used in core algorithms (yet). Discovered this with the new feature I added to dgml generation that also generates a 'Value' field to each state that is the pretty printed regex corresponding to the state with the given label and is shown as tool-tip in VS when hovering over the node. I found it very useful for debugging. I added a callback method to the state_graph that asks for the string value of a state, so the state_graph is still not dependent and knows nothing of regexes, it can be sued for anu other graphs.

@NikolajBjorner NikolajBjorner merged commit af54a79 into Z3Prover:master Sep 8, 2020
@veanes veanes deleted the fix4651 branch September 8, 2020 19:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants