[clang] 1e010c5 - Reland "[dataflow] avoid more accidental copies of Environment"

Sam McCall via cfe-commits cfe-commits at lists.llvm.org
Tue Jun 27 11:31:46 PDT 2023


Author: Sam McCall
Date: 2023-06-27T20:31:40+02:00
New Revision: 1e010c5c4fae43c52d6f5f1c8e8920c26bcc6cc7

URL: https://github.com/llvm/llvm-project/commit/1e010c5c4fae43c52d6f5f1c8e8920c26bcc6cc7
DIFF: https://github.com/llvm/llvm-project/commit/1e010c5c4fae43c52d6f5f1c8e8920c26bcc6cc7.diff

LOG: Reland "[dataflow] avoid more accidental copies of Environment"

This reverts commit fb13d027eae405a7be96fd7da0d72422e48a0719.

Added: 
    

Modified: 
    clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h
    clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp
    clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp

Removed: 
    


################################################################################
diff  --git a/clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h b/clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h
index 15e63c3d91a32..8494bc197cb25 100644
--- a/clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h
+++ b/clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h
@@ -226,9 +226,8 @@ class Environment {
   /// Requirements:
   ///
   ///  `Other` and `this` must use the same `DataflowAnalysisContext`.
-  LatticeJoinEffect join(const Environment &Other,
-                         Environment::ValueModel &Model);
-
+  Environment join(const Environment &Other,
+                   Environment::ValueModel &Model) const;
 
   /// Widens the environment point-wise, using `PrevEnv` as needed to inform the
   /// approximation.

diff  --git a/clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp b/clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp
index b2e67651626d5..3296fee39dd13 100644
--- a/clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp
+++ b/clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp
@@ -526,14 +526,12 @@ LatticeJoinEffect Environment::widen(const Environment &PrevEnv,
   return Effect;
 }
 
-LatticeJoinEffect Environment::join(const Environment &Other,
-                                    Environment::ValueModel &Model) {
+Environment Environment::join(const Environment &Other,
+                              Environment::ValueModel &Model) const {
   assert(DACtx == Other.DACtx);
   assert(ThisPointeeLoc == Other.ThisPointeeLoc);
   assert(CallStack == Other.CallStack);
 
-  auto Effect = LatticeJoinEffect::Unchanged;
-
   Environment JoinedEnv(*DACtx);
 
   JoinedEnv.CallStack = CallStack;
@@ -558,10 +556,8 @@ LatticeJoinEffect Environment::join(const Environment &Other,
     assert(Func != nullptr);
     if (Value *MergedVal =
             mergeDistinctValues(Func->getReturnType(), *ReturnVal, *this,
-                                *Other.ReturnVal, Other, JoinedEnv, Model)) {
+                                *Other.ReturnVal, Other, JoinedEnv, Model))
       JoinedEnv.ReturnVal = MergedVal;
-      Effect = LatticeJoinEffect::Changed;
-    }
   }
 
   if (ReturnLoc == Other.ReturnLoc)
@@ -574,19 +570,12 @@ LatticeJoinEffect Environment::join(const Environment &Other,
   // `DeclToLoc` and `Other.DeclToLoc` that map the same declaration to
   // 
diff erent storage locations.
   JoinedEnv.DeclToLoc = intersectDenseMaps(DeclToLoc, Other.DeclToLoc);
-  if (DeclToLoc.size() != JoinedEnv.DeclToLoc.size())
-    Effect = LatticeJoinEffect::Changed;
 
   JoinedEnv.ExprToLoc = intersectDenseMaps(ExprToLoc, Other.ExprToLoc);
-  if (ExprToLoc.size() != JoinedEnv.ExprToLoc.size())
-    Effect = LatticeJoinEffect::Changed;
 
   JoinedEnv.MemberLocToStruct =
       intersectDenseMaps(MemberLocToStruct, Other.MemberLocToStruct);
-  if (MemberLocToStruct.size() != JoinedEnv.MemberLocToStruct.size())
-    Effect = LatticeJoinEffect::Changed;
 
-  // FIXME: set `Effect` as needed.
   // FIXME: update join to detect backedges and simplify the flow condition
   // accordingly.
   JoinedEnv.FlowConditionToken = &DACtx->joinFlowConditions(
@@ -613,15 +602,10 @@ LatticeJoinEffect Environment::join(const Environment &Other,
             mergeDistinctValues(Loc->getType(), *Val, *this, *It->second, Other,
                                 JoinedEnv, Model)) {
       JoinedEnv.LocToVal.insert({Loc, MergedVal});
-      Effect = LatticeJoinEffect::Changed;
     }
   }
-  if (LocToVal.size() != JoinedEnv.LocToVal.size())
-    Effect = LatticeJoinEffect::Changed;
 
-  *this = std::move(JoinedEnv);
-
-  return Effect;
+  return JoinedEnv;
 }
 
 StorageLocation &Environment::createStorageLocation(QualType Type) {

diff  --git a/clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp b/clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp
index 1f23b6cf238f7..880ad6fa8bcf6 100644
--- a/clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp
+++ b/clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp
@@ -221,6 +221,59 @@ class PrettyStackTraceCFGElement : public llvm::PrettyStackTraceEntry {
   const char *Message;
 };
 
+// Builds a joined TypeErasedDataflowAnalysisState from 0 or more sources,
+// each of which may be owned (built as part of the join) or external (a
+// reference to an Environment that will outlive the builder).
+// Avoids unneccesary copies of the environment.
+class JoinedStateBuilder {
+  AnalysisContext ∾
+  std::optional<TypeErasedDataflowAnalysisState> OwnedState;
+  // Points either to OwnedState, an external Environment, or nothing.
+  const TypeErasedDataflowAnalysisState *CurrentState = nullptr;
+
+public:
+  JoinedStateBuilder(AnalysisContext &AC) : AC(AC) {}
+
+  void addOwned(TypeErasedDataflowAnalysisState State) {
+    if (!CurrentState) {
+      OwnedState = std::move(State);
+      CurrentState = &*OwnedState;
+    } else if (!OwnedState) {
+      OwnedState.emplace(std::move(CurrentState->Lattice),
+                         CurrentState->Env.join(State.Env, AC.Analysis));
+      AC.Analysis.joinTypeErased(OwnedState->Lattice, State.Lattice);
+    } else {
+      OwnedState->Env = CurrentState->Env.join(State.Env, AC.Analysis);
+      AC.Analysis.joinTypeErased(OwnedState->Lattice, State.Lattice);
+    }
+  }
+  void addUnowned(const TypeErasedDataflowAnalysisState &State) {
+    if (!CurrentState) {
+      CurrentState = &State;
+    } else if (!OwnedState) {
+      OwnedState.emplace(CurrentState->Lattice,
+                         CurrentState->Env.join(State.Env, AC.Analysis));
+      AC.Analysis.joinTypeErased(OwnedState->Lattice, State.Lattice);
+    } else {
+      OwnedState->Env = CurrentState->Env.join(State.Env, AC.Analysis);
+      AC.Analysis.joinTypeErased(OwnedState->Lattice, State.Lattice);
+    }
+  }
+  TypeErasedDataflowAnalysisState take() && {
+    if (!OwnedState) {
+      if (CurrentState)
+        OwnedState.emplace(CurrentState->Lattice, CurrentState->Env.fork());
+      else
+        // FIXME: Consider passing `Block` to Analysis.typeErasedInitialElement
+        // to enable building analyses like computation of dominators that
+        // initialize the state of each basic block 
diff erently.
+        OwnedState.emplace(AC.Analysis.typeErasedInitialElement(),
+                           AC.InitEnv.fork());
+    }
+    return std::move(*OwnedState);
+  }
+};
+
 } // namespace
 
 /// Computes the input state for a given basic block by joining the output
@@ -267,9 +320,7 @@ computeBlockInputState(const CFGBlock &Block, AnalysisContext &AC) {
     }
   }
 
-  std::optional<TypeErasedDataflowAnalysisState> MaybeState;
-
-  auto &Analysis = AC.Analysis;
+  JoinedStateBuilder Builder(AC);
   for (const CFGBlock *Pred : Preds) {
     // Skip if the `Block` is unreachable or control flow cannot get past it.
     if (!Pred || Pred->hasNoReturnElement())
@@ -282,36 +333,30 @@ computeBlockInputState(const CFGBlock &Block, AnalysisContext &AC) {
     if (!MaybePredState)
       continue;
 
-    TypeErasedDataflowAnalysisState PredState = MaybePredState->fork();
-    if (Analysis.builtinOptions()) {
+    if (AC.Analysis.builtinOptions()) {
       if (const Stmt *PredTerminatorStmt = Pred->getTerminatorStmt()) {
+        // We have a terminator: we need to mutate an environment to describe
+        // when the terminator is taken. Copy now.
+        TypeErasedDataflowAnalysisState Copy = MaybePredState->fork();
+
         const StmtToEnvMap StmtToEnv(AC.CFCtx, AC.BlockStates);
         auto [Cond, CondValue] =
-            TerminatorVisitor(StmtToEnv, PredState.Env,
+            TerminatorVisitor(StmtToEnv, Copy.Env,
                               blockIndexInPredecessor(*Pred, Block))
                 .Visit(PredTerminatorStmt);
         if (Cond != nullptr)
           // FIXME: Call transferBranchTypeErased even if BuiltinTransferOpts
           // are not set.
-          Analysis.transferBranchTypeErased(CondValue, Cond, PredState.Lattice,
-                                            PredState.Env);
+          AC.Analysis.transferBranchTypeErased(CondValue, Cond, Copy.Lattice,
+                                               Copy.Env);
+        Builder.addOwned(std::move(Copy));
+        continue;
       }
     }
-
-    if (MaybeState) {
-      Analysis.joinTypeErased(MaybeState->Lattice, PredState.Lattice);
-      MaybeState->Env.join(PredState.Env, Analysis);
-    } else {
-      MaybeState = std::move(PredState);
-    }
-  }
-  if (!MaybeState) {
-    // FIXME: Consider passing `Block` to `Analysis.typeErasedInitialElement()`
-    // to enable building analyses like computation of dominators that
-    // initialize the state of each basic block 
diff erently.
-    MaybeState.emplace(Analysis.typeErasedInitialElement(), AC.InitEnv.fork());
+    Builder.addUnowned(*MaybePredState);
+    continue;
   }
-  return std::move(*MaybeState);
+  return std::move(Builder).take();
 }
 
 /// Built-in transfer function for `CFGStmt`.


        


More information about the cfe-commits mailing list