Skip to content

Rfc sat backend mergesat #6088

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.

Sign up for GitHub

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

Closed
wants to merge 3 commits into from
Closed

Rfc sat backend mergesat #6088

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
2e6ca86
build: drop Werror
conp-solutions May 5, 2021
f4f40ed
solvers,sat: use MergeSat instead of Minisat2
conp-solutions May 5, 2021
8d60dae
sat,solvers: adapt setPolarity for MergeSat
conp-solutions May 6, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,7 @@ if("${CMAKE_CXX_COMPILER_ID}" STREQUAL "Clang" OR
set(CMAKE_CXX_FLAGS_RELEASE "-O2")
set(CMAKE_CXX_FLAGS_RELWITHDEBINFO "-O2 -g")
# Enable lots of warnings
set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -Wall -Wpedantic -Werror -Wno-deprecated-declarations -Wswitch-enum")
set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -Wall -Wpedantic -Wno-deprecated-declarations -Wswitch-enum")
Copy link
Contributor

Choose a reason for hiding this comment

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

The policy of the current build is to error on warnings, this should probably be discussed independently of other PRs. If this is required for this PR to work, I would recommend explaining why the warnings cannot be avoided.

elseif("${CMAKE_CXX_COMPILER_ID}" STREQUAL "MSVC")
# This would be the place to enable warnings for Windows builds, although
# config.inc doesn't seem to do that currently
Expand Down
13 changes: 6 additions & 7 deletions src/Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -119,19 +119,18 @@ DOWNLOADER = curl -L --remote-name
TAR = tar

minisat2-download:
@echo "Downloading Minisat 2.2.1"
@echo "Downloading MergeSat (replacing Minisat 2.2.1)"
@for i in $$(seq 1 3) ; do \
$(DOWNLOADER) \
http://ftp.debian.org/debian/pool/main/m/minisat2/minisat2_2.2.1.orig.tar.gz && \
https://github.com/conp-solutions/mergesat/archive/refs/tags/v3.0.tar.gz && \
exit 0 ; \
$(RM) minisat2_2.2.1.orig.tar.gz ; \
$(RM) v3.0.tar.gz ; \
if [ $$i -lt 3 ] ; then echo "Re-trying in 10 seconds" 1>&2 ; sleep 10 ; fi ; \
done ; exit 1
@$(TAR) xfz minisat2_2.2.1.orig.tar.gz
@$(TAR) xfz v3.0.tar.gz # will create mergesat-3.0
@rm -Rf ../minisat-2.2.1
@mv minisat2-2.2.1 ../minisat-2.2.1
@(cd ../minisat-2.2.1; patch -p1 < ../scripts/minisat-2.2.1-patch)
@rm minisat2_2.2.1.orig.tar.gz
@mv mergesat-3.0 ../minisat-2.2.1
@rm v3.0.tar.gz
Comment on lines +122 to +133
Copy link
Contributor

Choose a reason for hiding this comment

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

This appears to be editing and replacing the current MiniSAT build process. I would much prefer to see a new build path for MergeSat (like the ones for CaDiCaL or glucose) and then if this has much improved performance we can simply change the default. Also see recent PRs for documenting and updating alternative build systems (e.g. #6075 #6047).

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes; absolutely.


cudd-download:
@echo "Downloading Cudd 3.0.0"
Expand Down
2 changes: 1 addition & 1 deletion src/config.inc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ BUILD_ENV = AUTO
ifeq ($(BUILD_ENV),MSVC)
#CXXFLAGS += /Wall /WX
else
CXXFLAGS += -Wall -pedantic -Werror -Wno-deprecated-declarations -Wswitch-enum
CXXFLAGS += -Wall -pedantic -Wno-deprecated-declarations -Wswitch-enum
Copy link
Contributor

Choose a reason for hiding this comment

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

Again, this should be separate.

endif

ifeq ($(CPROVER_WITH_PROFILING),1)
Expand Down
11 changes: 10 additions & 1 deletion src/solvers/Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ endif
ifneq ($(MINISAT2),)
MINISAT2_SRC=sat/satcheck_minisat2.cpp
MINISAT2_INCLUDE=-I $(MINISAT2)
MINISAT2_LIB=$(MINISAT2)/minisat/simp/SimpSolver$(OBJEXT) $(MINISAT2)/minisat/core/Solver$(OBJEXT)
MINISAT2_LIB=$(MINISAT2)/minisat/simp/SimpSolver$(OBJEXT) $(MINISAT2)/minisat/core/Solver$(OBJEXT) $(MINISAT2)/minisat/utils/ccnr$(OBJEXT) $(MINISAT2)/minisat/utils/Options$(OBJEXT) $(MINISAT2)/minisat/utils/System$(OBJEXT)
Copy link
Contributor

Choose a reason for hiding this comment

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

Again, editing the MiniSAT and not providing a new option.

CP_CXXFLAGS += -DHAVE_MINISAT2 -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS
CLEANFILES += $(MINISAT2_LIB) $(patsubst %$(OBJEXT), %$(DEPEXT), $(MINISAT2_LIB))
endif
Expand Down Expand Up @@ -201,6 +201,15 @@ ifeq ($(BUILD_ENV_),MSVC)
sat/satcheck_minisat2$(OBJEXT): sat/satcheck_minisat2.cpp
$(CXX) $(CP_CXXFLAGS) /w /nologo /c /EHsc $< /Fo$@

$(MINISAT2)/minisat/utils/ccnr$(OBJEXT): $(MINISAT2)/minisat/utils/ccnr.cc
$(CXX) $(CP_CXXFLAGS) /w /nologo /c /EHsc $< /Fo$@

$(MINISAT2)/minisat/utils/Options$(OBJEXT): $(MINISAT2)/minisat/utils/Options.cc
$(CXX) $(CP_CXXFLAGS) /w /nologo /c /EHsc $< /Fo$@

$(MINISAT2)/minisat/utils/System$(OBJEXT): $(MINISAT2)/minisat/utils/System.cc
$(CXX) $(CP_CXXFLAGS) /w /nologo /c /EHsc $< /Fo$@

Comment on lines +204 to +212
Copy link
Contributor

Choose a reason for hiding this comment

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

Again, editing.

$(MINISAT2)/minisat/simp/SimpSolver$(OBJEXT): $(MINISAT2)/minisat/simp/SimpSolver.cc
$(CXX) $(CP_CXXFLAGS) /w /nologo /c /EHsc $< /Fo$@

Expand Down
2 changes: 1 addition & 1 deletion src/solvers/sat/satcheck_minisat2.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,7 @@ void satcheck_minisat2_baset<T>::set_polarity(literalt a, bool value)
try
{
add_variables();
solver->setPolarity(a.var_no(), value ? l_True : l_False);
solver->setPolarity(a.var_no(), value); // ? l_True : l_False);
}
catch(Minisat::OutOfMemoryException)
{
Expand Down