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

multiplication is variadic #341

Merged
merged 2 commits into from
Dec 12, 2023
Merged

Conversation

ffrohn
Copy link
Contributor

@ffrohn ffrohn commented Dec 11, 2023

This allows for creating multiplication-terms with more than two arguments with Z3. Currently, such terms can be created with CVC5, but not with Z3 (I didn't try other solvers).

Signed-off-by: Florian Frohn <[email protected]>
@yoni206 yoni206 self-requested a review December 12, 2023 07:03
@yoni206 yoni206 self-assigned this Dec 12, 2023
@yoni206
Copy link
Collaborator

yoni206 commented Dec 12, 2023

Thanks! Could you please include a test for this new feature?
Perhaps the best place would be to add a test here?
https://github.com/stanford-centaur/smt-switch/blob/master/tests/test-int.cpp

This would also clarify the status with the other solvers.

Signed-off-by: Florian Frohn <[email protected]>
{
mult = s->make_term(Mult, {two, three, four});
}
catch (const IncorrectUsageException &e)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Perhaps we could try this without catching? This will tell us what solvers have problems with that, and then we can fix it for those solvers?

Ideally we would like to be uniform w.r.t. back-end solvers, as much as possible.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure what you mean. If an exception is thrown, then the FAIL in the catch-block is executed, and the solver's error message is shown:

FAIL() << "creating mult-term failed: " << e.what();

For example, if I execute the test in the master branch on my machine, then I get:

$SMT_SWITCH/tests/test-int.cpp:58: Failure
Failed
creating mult-term failed: Can't apply * to 3 terms.
[  FAILED  ] ParameterizedSolverIntTests/IntTests.Mult/4, where GetParam() = Z3(logging=0) (1 ms)

Since all CI checks pass with my fix, I guess that Z3 is the only solver that can't handle variadic multiplication.

I guess it would also work as desired without catching (I'm not very familiar with gtest and how it handles exceptions), but then we cannot provide a custom error message ("creating mult-term failed: "...).

Copy link
Collaborator

Choose a reason for hiding this comment

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

Sorry, you are right, I missed the fact that the message goes to FAIL(). So if z3 is the only solver for which this does not work, could you perhaps try to make it work for that too? It should support it so it is unclear why this does not work:

{ Mult, Z3_mk_mul },

Copy link
Contributor Author

Choose a reason for hiding this comment

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

With my fix (a757315), it does work with Z3 (which is the reason why the CI checks passed after my last commit 61b4855). The problem was that z3_solver only tries to build a variadic term if the corresponding operation is marked as variadic in ops.h, which was not the case for Mult.

Copy link
Collaborator

@yoni206 yoni206 left a comment

Choose a reason for hiding this comment

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

Thanks for this change and for the discussion!

@yoni206 yoni206 merged commit d6a5b13 into stanford-centaur:master Dec 12, 2023
10 checks passed
rachelcleaveland pushed a commit to rachelcleaveland/smt-switch that referenced this pull request Jan 12, 2024
This allows for creating multiplication-terms with more than two arguments.
CyanoKobalamyne pushed a commit to CyanoKobalamyne/smt-switch that referenced this pull request May 16, 2024
This allows for creating multiplication-terms with more than two arguments.
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.

2 participants