[PATCH] D116370: [clang][dataflow] Add multi-variable constant propagation example.

[Stanislav Gatev via Phabricator via cfe-commits]

sgatev added inline comments.


================
Comment at: clang/unittests/Analysis/FlowSensitive/MultiVarConstantPropagationTest.cpp:9
+//
+// This file defines a simplistic version of Constant Propagation as an example
+// of a forward, monotonic dataflow analysis. The analysis tracks all
----------------
Indent two spaces.


================
Comment at: clang/unittests/Analysis/FlowSensitive/MultiVarConstantPropagationTest.cpp:160
+                        : ValueLattice::top();
+        return Vars;
+      }
----------------
Consider removing this `return` and moving the assignment that follows in an `else` block. I think this way the two cases (with and without initializer) will be more apparent.


================
Comment at: clang/unittests/Analysis/FlowSensitive/MultiVarConstantPropagationTest.cpp:162
+      }
+      Vars[Var] = ValueLattice::top();
+      return Vars;
----------------
I believe this should be `bottom` as `ValueLattice::bottom` models an undefined value.


================
Comment at: clang/unittests/Analysis/FlowSensitive/MultiVarConstantPropagationTest.cpp:171-173
+      Vars[Var] = (E->EvaluateAsInt(R, Context) && R.Val.isInt())
+                      ? ValueLattice(R.Val.getInt().getExtValue())
+                      : ValueLattice::top();
----------------
I think this makes the transfer function non-monotonic. Concretely, `Vars[var]` can be `top` and the right-hand side can be something else. The transfer function must be monotonic to guarantee termination. Perhaps this should be `Vars[Var].join(...)`?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D116370/new/

https://reviews.llvm.org/D116370