Make SolveEq and PlusCombineLikeTerms idempotent (#4438)
authorAndres Noetzli <andres.noetzli@gmail.com>
Tue, 19 May 2020 18:31:42 +0000 (11:31 -0700)
committerGitHub <noreply@github.com>
Tue, 19 May 2020 18:31:42 +0000 (11:31 -0700)
commiteebfe6389321c24329d9b58f699ad67486cc30e0
treebad5241cde1df28cc61cd6a596eafa6fd28a6f15
parent3b50dfe623f44e97d85449fa2a7566e81c639b47
Make SolveEq and PlusCombineLikeTerms idempotent (#4438)

Fixes #3692 and an assertion failure that came up during the test runs
for SMT-COMP. The bit-vector rewrites `SolveEq` and
`PlusCombineLikeTerms` were not always idempotent. At a high level,
`SolveEq` combines common terms from two sides of an equality and
`PlusCombineLikeTerms` combines common terms within an addition.
However, in doing so, these rewrites were reordering the operands of the
bit-vector addition based on the node ids of the terms that were
multiplied with their coefficients. Consider the addition `3 * x * y + 5
* y * z` (the bit-width does not matter). `PlusCombineLikeTerms` would
reorder this addition to `5 * y * z + 3 * x * y` if the node id of `y *
z` was smaller than the node id of `x * y`. The issue is that node ids
are not fixed for a given term: If we have a term `x * y` and that term
reaches ref count 0, we may get a different id for that same term if we
recreate it later on. When terms reach ref count 0, we don't immediately
delete them but add them to our set of zombies to be deleted whenever
the list of zombies grows larger than some fixed size. When applying
`SolveEq` and `PlusCombineLikeTerms` multiple times (even in direct
succession without doing anything else), the node order may change
because some of the terms like `x * y` may be zombies while others have
been deleted and get new ids, leading to the relative order of node ids
changing. I suspect that we could construct a case where we get into an
infinite rewrite loop.

This commit addresses the issue as follows: It does not perform the
rewrites `SolveEq` and `PlusCombineLikeTerms` if none of the operands
change. This makes the rewrites idempotent. Note however that we are
still not guaranteeing that a term has the same rewritten form
throughout an execution because the node ids may change if the term has
been freed in the meantime. However, this limitation is consistent with
other rewrites such as the reordering of equalities.

I am including the minimized test case from our run on SMT-LIB. I am
ommittin the test case from #3692 because I couldn't trigger it on
master (not surprising since the issue requires very specific
circumstances to actually occur). However, I was able to reproduce the
issue on the CVC4 version mentioned in the issue and confirmed that this
fix worked for that older version.
src/theory/bv/theory_bv_rewrite_rules_normalization.h
test/regress/CMakeLists.txt
test/regress/regress2/bv/opStructure_MBA_6.scrambled.min.smt2 [new file with mode: 0644]