This is an archive of the discontinued LLVM Phabricator instance.

[clang][dataflow] Add explicit "AST" nodes for implications and iff
ClosedPublic

Authored by gribozavr on Jul 25 2022, 2:32 PM.

Details

Summary

Previously we used to desugar implications and biconditionals into
equivalent CNF/DNF as soon as possible. However, this desugaring makes
debug output (Environment::dump()) less readable than it could be.
Therefore, it makes sense to keep the sugared representation of a
boolean formula, and desugar it in the solver.

Diff Detail

Event Timeline

gribozavr created this revision.Jul 25 2022, 2:32 PM
Herald added a project: Restricted Project. · View Herald Transcript
gribozavr requested review of this revision.Jul 25 2022, 2:32 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 25 2022, 2:32 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript
gribozavr updated this revision to Diff 447484.Jul 25 2022, 3:00 PM

Normalized more comments

xazax.hun added inline comments.Jul 25 2022, 3:18 PM
clang/lib/Analysis/FlowSensitive/WatchedLiteralsSolver.cpp
354

I wonder why this simplification is done here only for BiconditionalValue. Other operations seem to not do these sorts of simplifications. Also, when would we take this branch? getOrCreateIff has a special case when both operands are the same.

xazax.hun added inline comments.Jul 25 2022, 3:20 PM
clang/lib/Analysis/FlowSensitive/WatchedLiteralsSolver.cpp
354

Oh, looking at the other patch I see it mentioning desugaring. So can desugaring also introduce Biconditionals?

gribozavr2 added inline comments.
clang/lib/Analysis/FlowSensitive/WatchedLiteralsSolver.cpp
354

It is also necessary in the other cases due to the precondition of addClause(), I'm adding that special handling in a separate patch: https://reviews.llvm.org/D130522

We would take this branch when someone avoids using the DataflowContext API. So not extremely likely at the moment, but it is a latent bug that can be exposed by some future refactoring.

xazax.hun accepted this revision.Jul 25 2022, 3:26 PM
This revision is now accepted and ready to land.Jul 25 2022, 3:26 PM
xazax.hun added inline comments.Jul 25 2022, 3:26 PM
clang/lib/Analysis/FlowSensitive/WatchedLiteralsSolver.cpp
354

I see, thanks!

sgatev accepted this revision.Jul 26 2022, 12:45 AM
sgatev added inline comments.
clang/include/clang/Analysis/FlowSensitive/Value.h
39–40
clang/lib/Analysis/FlowSensitive/DebugSupport.cpp
110

I think <=> would be more natural.

clang/unittests/Analysis/FlowSensitive/SolverTest.cpp
219

Let's add a label: !((X <=> Y) <=> ((X ^ Y) v (!X ^ !Y)))

322

Let's add a label: !((X => Y) <=> (!X v Y))

wyt accepted this revision.Jul 26 2022, 1:47 AM
gribozavr2 added inline comments.Jul 26 2022, 5:17 AM
clang/include/clang/Analysis/FlowSensitive/Value.h
39–40

Applied.

clang/lib/Analysis/FlowSensitive/DebugSupport.cpp
110

Yeah, but = is what smtlib uses.

clang/lib/Analysis/FlowSensitive/WatchedLiteralsSolver.cpp
354

Marking done.

clang/unittests/Analysis/FlowSensitive/SolverTest.cpp
219

Added!

322

Added!

This revision was landed with ongoing or failed builds.Jul 26 2022, 5:19 AM
This revision was automatically updated to reflect the committed changes.