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

Drop clause if it contains an impossible BinaryConstraint #2376

Merged

Conversation

adamjseitz
Copy link
Contributor

Some BinaryConstraint atoms can be determined to be impossible during synthesis. This PR introduces a transform to eliminate clauses containing such impossible atoms.

These kinds of atoms are not likely to be written directly, but can easily arise with inlined code.

In ddisasm (with a small work-in-progress change to take greater advantage of this), across its various arch configurations, Souffle generates between 5.0% and 12% fewer lines of C++ with this patch. This yields build time improvements of 2.5% - 10% depending on arch configuration and compiler.

This is my first attempt at adding a transformer pass, so please apply additional scrutiny.

Some BinaryConstraint atoms can be determined to be impossible during
synthesis. Introduce a transform to eliminate clauses containing such
impossible atoms.

These kinds of atoms are not likely to be written directly, but can
easily arise with inlined code.
@codecov
Copy link

codecov bot commented Dec 20, 2022

Codecov Report

Merging #2376 (fc3eb44) into master (251e8fd) will increase coverage by 0.00%.
The diff coverage is 95.83%.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #2376   +/-   ##
=======================================
  Coverage   77.52%   77.53%           
=======================================
  Files         467      469    +2     
  Lines       30778    30833   +55     
=======================================
+ Hits        23862    23906   +44     
- Misses       6916     6927   +11     
Impacted Files Coverage Δ
.../ast/transform/SimplifyConstantBinaryConstraints.h 50.00% <50.00%> (ø)
src/MainDriver.cpp 70.35% <100.00%> (+0.11%) ⬆️
...st/transform/SimplifyConstantBinaryConstraints.cpp 100.00% <100.00%> (ø)
src/ast/BooleanConstraint.cpp 70.00% <0.00%> (-30.00%) ⬇️
src/ast/analysis/ClauseNormalisation.cpp 83.33% <0.00%> (-7.41%) ⬇️
src/ram/utility/Visitor.h 53.78% <0.00%> (-0.76%) ⬇️
src/ast/transform/SemanticChecker.cpp 97.04% <0.00%> (-0.01%) ⬇️
src/ram/analysis/Complexity.cpp 100.00% <0.00%> (ø)
src/ram/transform/ReorderConditions.cpp 100.00% <0.00%> (ø)
src/ast/analysis/typesystem/Type.cpp 47.43% <0.00%> (+0.59%) ⬆️

Copy link
Member

@quentin quentin left a comment

Choose a reason for hiding this comment

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

Could this be done more generally by introducing a constant folding transformer that runs before the RemoveBooleanConstraintsTransformer ?

The RemoveBooleanConstraintsTransformer removes clauses that contain the false constraint.

The constant folding transformer would transform non-satisfiable binary constraints into the false BinaryConstraint, and always-satisfiable binary constraints into the true BinaryConstraint.

Other constant folding operations could be performed as well.

@adamjseitz
Copy link
Contributor Author

Could this be done more generally by introducing a constant folding transformer that runs before the RemoveBooleanConstraintsTransformer ?

The RemoveBooleanConstraintsTransformer removes clauses that contain the false constraint.

The constant folding transformer would transform non-satisfiable binary constraints into the false BinaryConstraint, and always-satisfiable binary constraints into the true BinaryConstraint.

Other constant folding operations could be performed as well.

That sounds to me like an approach that would make sense, although we'd have to do some tweaking of the transform pipeline order to get everything working as desired. As I noted, these kind of impossible constraints arise most often in inlined relations. I was only able to get my changes to have the desired result with it after both InlineRelationsTransformer and ResolveAliasesTransformer in the pipeline. RemoveBooleanConstraintsTransformer currently runs before inlining in the pipeline.

Do you think it would make sense to move RemoveBooleanConstraintsTransformer until after inlining in the pipeline? Or should it be run again, similar to ResolveAliasesTransformer, which runs several times?

@adamjseitz
Copy link
Contributor Author

Hi @quentin,

I pushed another commit to refactor the new transform as SimplifyBinaryConstraintsTransform, which just rewrites BinaryConstraint into BooleanConstrant, and added another pass of RemoveBooleanConstraintsTransformer after it runs.

Of course, we could support rewriting more BinaryConstraint operand types, but for now I've just left it supporting EQ and NE .

Let me know if you have further feedback!

namespace souffle::ast::transform {

/**
* Eliminate clauses containing unsatisfiable constant binary constraints
Copy link
Member

Choose a reason for hiding this comment

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

this transform does not eliminate clauses, it simplifies binary constraints into boolean constraints.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch, updated the docstring.

@adamjseitz adamjseitz force-pushed the eliminate-constant-binary-constraint branch from 678d1aa to fc3eb44 Compare January 13, 2023 14:24
@quentin quentin merged commit 0d75dc3 into souffle-lang:master Jan 16, 2023
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