[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