Skip to content

Optimization: Share opposite-polarity literals in clause code trees - #880

Open
Synrom wants to merge 16 commits into
vprover:masterfrom
Synrom:opposite-optimization
Open

Optimization: Share opposite-polarity literals in clause code trees#880
Synrom wants to merge 16 commits into
vprover:masterfrom
Synrom:opposite-optimization

Conversation

@Synrom

@Synrom Synrom commented Jul 16, 2026

Copy link
Copy Markdown

This PR optimizes clause code tree subsumption resolution by sharing literal information across polarities. Instead of storing separate positive and negative LitInfo entries for each literal, matching records whether a result came from the opposite polarity and handles that during matcher execution.

The implementation preserves subsumption priority over subsumption resolution by keeping non-opposite matches first in ILStruct and making ClauseMatcher::matchGlobalVars try non-opposite matches before falling back to opposite-polarity matches.

This optimization is currently not compatible with CHECK_GROUND_TERM=1.

Comment thread Indexing/ClauseCodeTree.cpp
Comment thread Indexing/ClauseCodeTree.cpp Outdated
Comment thread Kernel/FlatTerm.hpp Outdated
Comment thread Kernel/FlatTerm.hpp
inline bool isFun(unsigned num) const { return isFun() && _number()==num; }
/* Litearl headers encode polarity in their lowest bit, so xor-ing with 1
* gives the opposite (negated) predicate. Only meaningful for literal predicates. */
inline bool isOppositeFun(unsigned num) const { return isFun() && (_number()^1)==num; }

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Would it make sense to assert that this actually gives the opposite literal, so if we ever change it, it automatically triggers? Maybe not here, since this is called very often.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

In the method itself, it would be hard. FlatTerm::changeLiteralPolarity uses the same trick, so you could add an assert there.
But as far as I can tell, FlatTerm::changeLiteralPolarity will not be executed anymore.
Maybe, I can just add a unit test that checks the FlatTerm behavior against the Literal implementation?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Okay, the unit test would be awesome! Please feel free to remove any unused stuff from FlatTerm too.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

I've added a unit test now :) should I also remove the now-unused opposite field and related methods from LitInfo?
The methods are the only remaining callers of FlatTerm::changeLiteralPolarity, so that could go to.

Comment thread Indexing/CodeTree.hpp Outdated
Comment thread Indexing/CodeTree.hpp
Comment thread Indexing/CodeTree.hpp Outdated
Comment thread Indexing/CodeTree.cpp Outdated
Comment thread Indexing/CodeTree.cpp
Comment thread Indexing/CodeTree.cpp Outdated
Comment thread Indexing/CodeTree.cpp Outdated
Comment thread Indexing/CodeTree.cpp
@mezpusz

mezpusz commented Aug 24, 2026

Copy link
Copy Markdown
Contributor

Nice work, once the conflict is resolved I will run some more tests to be sure. Do I get it right that this shouldn't affect anything if fsr=off? Maybe that could be an extra check to see that no bugs are introduced.

@Synrom

Synrom commented Aug 25, 2026

Copy link
Copy Markdown
Author

Thank you :) Yes with fsr=off, it should be identical. I ran:

./vampire  -sa discount -fsr off -al 1000 --random_seed 1 -t 0 ../../problems/condensed_detachment_4.p

On master and on opposite-optimization and the runtime statistics (clauses created, clauses deleted, ssat_new_components, ...) were exactly identical.

@mezpusz

mezpusz commented Aug 25, 2026

Copy link
Copy Markdown
Contributor

This optimization is currently not compatible with CHECK_GROUND_TERM=1.

Could you please add an ASSERTION_VIOLATION and a TODO referring to this PR somewhere so that whenever we set CHECK_GROUND_TERM=1 we notice this? Otherwise I think removing the feature as well as fixing it for this optimization is unnecessary in this PR.

@Synrom

Synrom commented Aug 25, 2026

Copy link
Copy Markdown
Author

Alright, I've added an assertion plus TODO now.

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