[PATCH] D116834: [clang][dataflow] Change `transfer` function to update lattice element in place.

[Yitzhak Mandelbaum via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rG64f7b2d4bf92: [clang][dataflow] Change `transfer` function 
to update lattice element in place. (authored by ymandel).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116834

Files:
  clang/include/clang/Analysis/FlowSensitive/DataflowAnalysis.h
  clang/include/clang/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.h
  clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp
  clang/unittests/Analysis/FlowSensitive/MultiVarConstantPropagationTest.cpp
  clang/unittests/Analysis/FlowSensitive/NoopAnalysis.h
  clang/unittests/Analysis/FlowSensitive/SingleVarConstantPropagationTest.cpp
  clang/unittests/Analysis/FlowSensitive/TypeErasedDataflowAnalysisTest.cpp

Index: clang/unittests/Analysis/FlowSensitive/TypeErasedDataflowAnalysisTest.cpp
===
--- clang/unittests/Analysis/FlowSensitive/TypeErasedDataflowAnalysisTest.cpp
+++ clang/unittests/Analysis/FlowSensitive/TypeErasedDataflowAnalysisTest.cpp
@@ -113,9 +113,8 @@
 
   static NonConvergingLattice initialElement() { return {0}; }
 
-  NonConvergingLattice transfer(const Stmt *S, const NonConvergingLattice ,
-Environment ) {
-return {E.State + 1};
+  void transfer(const Stmt *S, NonConvergingLattice , Environment ) {
+++E.State;
   }
 };
 
@@ -165,15 +164,12 @@
 
   static FunctionCallLattice initialElement() { return {}; }
 
-  FunctionCallLattice transfer(const Stmt *S, const FunctionCallLattice ,
-   Environment ) {
-FunctionCallLattice R = E;
+  void transfer(const Stmt *S, FunctionCallLattice , Environment ) {
 if (auto *C = dyn_cast(S)) {
   if (auto *F = dyn_cast(C->getCalleeDecl())) {
-R.CalledFunctions.insert(F->getNameInfo().getAsString());
+E.CalledFunctions.insert(F->getNameInfo().getAsString());
   }
 }
-return R;
   }
 };
 
Index: clang/unittests/Analysis/FlowSensitive/SingleVarConstantPropagationTest.cpp
===
--- clang/unittests/Analysis/FlowSensitive/SingleVarConstantPropagationTest.cpp
+++ clang/unittests/Analysis/FlowSensitive/SingleVarConstantPropagationTest.cpp
@@ -121,9 +121,8 @@
 return ConstantPropagationLattice::bottom();
   }
 
-  ConstantPropagationLattice transfer(const Stmt *S,
-  const ConstantPropagationLattice ,
-  Environment ) {
+  void transfer(const Stmt *S, ConstantPropagationLattice ,
+Environment ) {
 auto matcher = stmt(
 anyOf(declStmt(hasSingleDecl(varDecl(hasType(isInteger()),
  hasInitializer(expr().bind(kInit)))
@@ -137,7 +136,7 @@
 ASTContext  = getASTContext();
 auto Results = match(matcher, *S, Context);
 if (Results.empty())
-  return Element;
+  return;
 const BoundNodes  = Results[0];
 
 const auto *Var = Nodes.getNodeAs(kVar);
@@ -145,30 +144,26 @@
 
 if (const auto *E = Nodes.getNodeAs(kInit)) {
   Expr::EvalResult R;
-  if (E->EvaluateAsInt(R, Context) && R.Val.isInt())
-return ConstantPropagationLattice{
-{{Var, R.Val.getInt().getExtValue()}}};
-  return ConstantPropagationLattice::top();
-}
-
-if (Nodes.getNodeAs(kJustAssignment)) {
+  Element =
+  (E->EvaluateAsInt(R, Context) && R.Val.isInt())
+  ? ConstantPropagationLattice{{{Var,
+ R.Val.getInt().getExtValue()}}}
+  : ConstantPropagationLattice::top();
+} else if (Nodes.getNodeAs(kJustAssignment)) {
   const auto *RHS = Nodes.getNodeAs(kRHS);
   assert(RHS != nullptr);
 
   Expr::EvalResult R;
-  if (RHS->EvaluateAsInt(R, Context) && R.Val.isInt())
-return ConstantPropagationLattice{
-{{Var, R.Val.getInt().getExtValue()}}};
-  return ConstantPropagationLattice::top();
-}
-
-// Any assignment involving the expression itself resets the variable to
-// "unknown". A more advanced analysis could try to evaluate the compound
-// assignment. For example, `x += 0` need not invalidate `x`.
-if (Nodes.getNodeAs(kAssignment))
-  return ConstantPropagationLattice::top();
-
-llvm_unreachable("expected at least one bound identifier");
+  Element =
+  (RHS->EvaluateAsInt(R, Context) && R.Val.isInt())
+  ? ConstantPropagationLattice{{{Var,
+ R.Val.getInt().getExtValue()}}}
+  : ConstantPropagationLattice::top();
+} else if (Nodes.getNodeAs(kAssignment))
+  // Any assignment involving the expression itself resets the variable to
+  // 

[PATCH] D116834: [clang][dataflow] Change `transfer` function to update lattice element in place.

[Yitzhak Mandelbaum via Phabricator via cfe-commits]

ymandel updated this revision to Diff 398612.
ymandel added a comment.

address comments


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116834

Files:
  clang/include/clang/Analysis/FlowSensitive/DataflowAnalysis.h
  clang/include/clang/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.h
  clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp
  clang/unittests/Analysis/FlowSensitive/MultiVarConstantPropagationTest.cpp
  clang/unittests/Analysis/FlowSensitive/NoopAnalysis.h
  clang/unittests/Analysis/FlowSensitive/SingleVarConstantPropagationTest.cpp
  clang/unittests/Analysis/FlowSensitive/TypeErasedDataflowAnalysisTest.cpp

Index: clang/unittests/Analysis/FlowSensitive/TypeErasedDataflowAnalysisTest.cpp
===
--- clang/unittests/Analysis/FlowSensitive/TypeErasedDataflowAnalysisTest.cpp
+++ clang/unittests/Analysis/FlowSensitive/TypeErasedDataflowAnalysisTest.cpp
@@ -113,9 +113,8 @@
 
   static NonConvergingLattice initialElement() { return {0}; }
 
-  NonConvergingLattice transfer(const Stmt *S, const NonConvergingLattice ,
-Environment ) {
-return {E.State + 1};
+  void transfer(const Stmt *S, NonConvergingLattice , Environment ) {
+++E.State;
   }
 };
 
@@ -165,15 +164,12 @@
 
   static FunctionCallLattice initialElement() { return {}; }
 
-  FunctionCallLattice transfer(const Stmt *S, const FunctionCallLattice ,
-   Environment ) {
-FunctionCallLattice R = E;
+  void transfer(const Stmt *S, FunctionCallLattice , Environment ) {
 if (auto *C = dyn_cast(S)) {
   if (auto *F = dyn_cast(C->getCalleeDecl())) {
-R.CalledFunctions.insert(F->getNameInfo().getAsString());
+E.CalledFunctions.insert(F->getNameInfo().getAsString());
   }
 }
-return R;
   }
 };
 
Index: clang/unittests/Analysis/FlowSensitive/SingleVarConstantPropagationTest.cpp
===
--- clang/unittests/Analysis/FlowSensitive/SingleVarConstantPropagationTest.cpp
+++ clang/unittests/Analysis/FlowSensitive/SingleVarConstantPropagationTest.cpp
@@ -121,9 +121,8 @@
 return ConstantPropagationLattice::bottom();
   }
 
-  ConstantPropagationLattice transfer(const Stmt *S,
-  const ConstantPropagationLattice ,
-  Environment ) {
+  void transfer(const Stmt *S, ConstantPropagationLattice ,
+Environment ) {
 auto matcher = stmt(
 anyOf(declStmt(hasSingleDecl(varDecl(hasType(isInteger()),
  hasInitializer(expr().bind(kInit)))
@@ -137,7 +136,7 @@
 ASTContext  = getASTContext();
 auto Results = match(matcher, *S, Context);
 if (Results.empty())
-  return Element;
+  return;
 const BoundNodes  = Results[0];
 
 const auto *Var = Nodes.getNodeAs(kVar);
@@ -145,30 +144,26 @@
 
 if (const auto *E = Nodes.getNodeAs(kInit)) {
   Expr::EvalResult R;
-  if (E->EvaluateAsInt(R, Context) && R.Val.isInt())
-return ConstantPropagationLattice{
-{{Var, R.Val.getInt().getExtValue()}}};
-  return ConstantPropagationLattice::top();
-}
-
-if (Nodes.getNodeAs(kJustAssignment)) {
+  Element =
+  (E->EvaluateAsInt(R, Context) && R.Val.isInt())
+  ? ConstantPropagationLattice{{{Var,
+ R.Val.getInt().getExtValue()}}}
+  : ConstantPropagationLattice::top();
+} else if (Nodes.getNodeAs(kJustAssignment)) {
   const auto *RHS = Nodes.getNodeAs(kRHS);
   assert(RHS != nullptr);
 
   Expr::EvalResult R;
-  if (RHS->EvaluateAsInt(R, Context) && R.Val.isInt())
-return ConstantPropagationLattice{
-{{Var, R.Val.getInt().getExtValue()}}};
-  return ConstantPropagationLattice::top();
-}
-
-// Any assignment involving the expression itself resets the variable to
-// "unknown". A more advanced analysis could try to evaluate the compound
-// assignment. For example, `x += 0` need not invalidate `x`.
-if (Nodes.getNodeAs(kAssignment))
-  return ConstantPropagationLattice::top();
-
-llvm_unreachable("expected at least one bound identifier");
+  Element =
+  (RHS->EvaluateAsInt(R, Context) && R.Val.isInt())
+  ? ConstantPropagationLattice{{{Var,
+ R.Val.getInt().getExtValue()}}}
+  : ConstantPropagationLattice::top();
+} else if (Nodes.getNodeAs(kAssignment))
+  // Any assignment involving the expression itself resets the variable to
+  // "unknown". A more advanced analysis could try to evaluate the compound
+  // assignment. For example, `x += 0` need not invalidate `x`.
+  Element = 

[PATCH] D116834: [clang][dataflow] Change `transfer` function to update lattice element in place.

[Stanislav Gatev via Phabricator via cfe-commits]

sgatev accepted this revision.
sgatev added inline comments.
This revision is now accepted and ready to land.



Comment at: 
clang/unittests/Analysis/FlowSensitive/TypeErasedDataflowAnalysisTest.cpp:167
 
-  FunctionCallLattice transfer(const Stmt *S, const FunctionCallLattice ,
-   Environment ) {
-FunctionCallLattice R = E;
+  void transfer(const Stmt *S, FunctionCallLattice , Environment ) {
 if (auto *C = dyn_cast(S)) {

nit: let's call this `E` for consistency.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116834

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D116834: [clang][dataflow] Change `transfer` function to update lattice element in place.

[Yitzhak Mandelbaum via Phabricator via cfe-commits]

ymandel created this revision.
ymandel added reviewers: sgatev, gribozavr2.
ymandel requested review of this revision.
Herald added a project: clang.

Currently, the transfer function returns a new lattice element, which forces an
unnecessary copy on processing each CFG statement.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D116834

Files:
  clang/include/clang/Analysis/FlowSensitive/DataflowAnalysis.h
  clang/include/clang/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.h
  clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp
  clang/unittests/Analysis/FlowSensitive/MultiVarConstantPropagationTest.cpp
  clang/unittests/Analysis/FlowSensitive/NoopAnalysis.h
  clang/unittests/Analysis/FlowSensitive/SingleVarConstantPropagationTest.cpp
  clang/unittests/Analysis/FlowSensitive/TypeErasedDataflowAnalysisTest.cpp

Index: clang/unittests/Analysis/FlowSensitive/TypeErasedDataflowAnalysisTest.cpp
===
--- clang/unittests/Analysis/FlowSensitive/TypeErasedDataflowAnalysisTest.cpp
+++ clang/unittests/Analysis/FlowSensitive/TypeErasedDataflowAnalysisTest.cpp
@@ -113,9 +113,8 @@
 
   static NonConvergingLattice initialElement() { return {0}; }
 
-  NonConvergingLattice transfer(const Stmt *S, const NonConvergingLattice ,
-Environment ) {
-return {E.State + 1};
+  void transfer(const Stmt *S, NonConvergingLattice , Environment ) {
+++E.State;
   }
 };
 
@@ -165,15 +164,12 @@
 
   static FunctionCallLattice initialElement() { return {}; }
 
-  FunctionCallLattice transfer(const Stmt *S, const FunctionCallLattice ,
-   Environment ) {
-FunctionCallLattice R = E;
+  void transfer(const Stmt *S, FunctionCallLattice , Environment ) {
 if (auto *C = dyn_cast(S)) {
   if (auto *F = dyn_cast(C->getCalleeDecl())) {
 R.CalledFunctions.insert(F->getNameInfo().getAsString());
   }
 }
-return R;
   }
 };
 
Index: clang/unittests/Analysis/FlowSensitive/SingleVarConstantPropagationTest.cpp
===
--- clang/unittests/Analysis/FlowSensitive/SingleVarConstantPropagationTest.cpp
+++ clang/unittests/Analysis/FlowSensitive/SingleVarConstantPropagationTest.cpp
@@ -121,9 +121,8 @@
 return ConstantPropagationLattice::bottom();
   }
 
-  ConstantPropagationLattice transfer(const Stmt *S,
-  const ConstantPropagationLattice ,
-  Environment ) {
+  void transfer(const Stmt *S, ConstantPropagationLattice ,
+Environment ) {
 auto matcher = stmt(
 anyOf(declStmt(hasSingleDecl(varDecl(hasType(isInteger()),
  hasInitializer(expr().bind(kInit)))
@@ -137,7 +136,7 @@
 ASTContext  = getASTContext();
 auto Results = match(matcher, *S, Context);
 if (Results.empty())
-  return Element;
+  return;
 const BoundNodes  = Results[0];
 
 const auto *Var = Nodes.getNodeAs(kVar);
@@ -145,30 +144,26 @@
 
 if (const auto *E = Nodes.getNodeAs(kInit)) {
   Expr::EvalResult R;
-  if (E->EvaluateAsInt(R, Context) && R.Val.isInt())
-return ConstantPropagationLattice{
-{{Var, R.Val.getInt().getExtValue()}}};
-  return ConstantPropagationLattice::top();
-}
-
-if (Nodes.getNodeAs(kJustAssignment)) {
+  Element =
+  (E->EvaluateAsInt(R, Context) && R.Val.isInt())
+  ? ConstantPropagationLattice{{{Var,
+ R.Val.getInt().getExtValue()}}}
+  : ConstantPropagationLattice::top();
+} else if (Nodes.getNodeAs(kJustAssignment)) {
   const auto *RHS = Nodes.getNodeAs(kRHS);
   assert(RHS != nullptr);
 
   Expr::EvalResult R;
-  if (RHS->EvaluateAsInt(R, Context) && R.Val.isInt())
-return ConstantPropagationLattice{
-{{Var, R.Val.getInt().getExtValue()}}};
-  return ConstantPropagationLattice::top();
-}
-
-// Any assignment involving the expression itself resets the variable to
-// "unknown". A more advanced analysis could try to evaluate the compound
-// assignment. For example, `x += 0` need not invalidate `x`.
-if (Nodes.getNodeAs(kAssignment))
-  return ConstantPropagationLattice::top();
-
-llvm_unreachable("expected at least one bound identifier");
+  Element =
+  (RHS->EvaluateAsInt(R, Context) && R.Val.isInt())
+  ? ConstantPropagationLattice{{{Var,
+ R.Val.getInt().getExtValue()}}}
+  : ConstantPropagationLattice::top();
+} else if (Nodes.getNodeAs(kAssignment))
+  // Any assignment involving the expression itself resets the variable to
+  // "unknown". A more advanced analysis could try to evaluate the compound
+  // assignment. For example, `x += 0` need not