[clang] 6f22de6 - [dataflow] Use consistent, symmetrical, non-mutating erased signature for join()
Sam McCall via cfe-commits
cfe-commits at lists.llvm.org
Wed Jun 28 02:30:32 PDT 2023
Author: Sam McCall
Date: 2023-06-28T11:30:16+02:00
New Revision: 6f22de67c585156aea6a4554f96315094092d211
URL: https://github.com/llvm/llvm-project/commit/6f22de67c585156aea6a4554f96315094092d211
DIFF: https://github.com/llvm/llvm-project/commit/6f22de67c585156aea6a4554f96315094092d211.diff
LOG: [dataflow] Use consistent, symmetrical, non-mutating erased signature for join()
Mutating join() isn't used and so appears to be an anti-optimization.
Having Lattice vs Environment inconsistent is awkward, particularly when trying
to minimize copies while joining.
This patch eliminates the difference, but doesn't actually change the signature
of join on concrete lattice types (as that's a breaking change).
Differential Revision: https://reviews.llvm.org/D153908
Added:
Modified:
clang/include/clang/Analysis/FlowSensitive/DataflowAnalysis.h
clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h
clang/include/clang/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.h
clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp
clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp
Removed:
################################################################################
diff --git a/clang/include/clang/Analysis/FlowSensitive/DataflowAnalysis.h b/clang/include/clang/Analysis/FlowSensitive/DataflowAnalysis.h
index b6ba3be053c8d..c6e9f848477f8 100644
--- a/clang/include/clang/Analysis/FlowSensitive/DataflowAnalysis.h
+++ b/clang/include/clang/Analysis/FlowSensitive/DataflowAnalysis.h
@@ -61,6 +61,7 @@ namespace dataflow {
/// argument by computing their least upper bound, modifies the object if
/// necessary, and returns an effect indicating whether any changes were
/// made to it;
+/// FIXME: make it `static LatticeT join(const LatticeT&, const LatticeT&)`
/// * `bool operator==(const LatticeT &) const` - returns true if and only if
/// the object is equal to the argument.
///
@@ -98,11 +99,13 @@ class DataflowAnalysis : public TypeErasedDataflowAnalysis {
return {static_cast<Derived *>(this)->initialElement()};
}
- LatticeJoinEffect joinTypeErased(TypeErasedLattice &E1,
+ TypeErasedLattice joinTypeErased(const TypeErasedLattice &E1,
const TypeErasedLattice &E2) final {
- Lattice &L1 = llvm::any_cast<Lattice &>(E1.Value);
+ // FIXME: change the signature of join() to avoid copying here.
+ Lattice L1 = llvm::any_cast<const Lattice &>(E1.Value);
const Lattice &L2 = llvm::any_cast<const Lattice &>(E2.Value);
- return L1.join(L2);
+ L1.join(L2);
+ return {std::move(L1)};
}
LatticeJoinEffect widenTypeErased(TypeErasedLattice &Current,
diff --git a/clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h b/clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h
index 8494bc197cb25..8da359880e3ce 100644
--- a/clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h
+++ b/clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h
@@ -218,16 +218,15 @@ class Environment {
bool equivalentTo(const Environment &Other,
Environment::ValueModel &Model) const;
- /// Joins the environment with `Other` by taking the intersection of storage
- /// locations and values that are stored in them. Distinct values that are
- /// assigned to the same storage locations in the environment and `Other` are
- /// merged using `Model`.
+ /// Joins two environments by taking the intersection of storage locations and
+ /// values that are stored in them. Distinct values that are assigned to the
+ /// same storage locations in `EnvA` and `EnvB` are merged using `Model`.
///
/// Requirements:
///
- /// `Other` and `this` must use the same `DataflowAnalysisContext`.
- Environment join(const Environment &Other,
- Environment::ValueModel &Model) const;
+ /// `EnvA` and `EnvB` must use the same `DataflowAnalysisContext`.
+ static Environment join(const Environment &EnvA, const Environment &EnvB,
+ Environment::ValueModel &Model);
/// Widens the environment point-wise, using `PrevEnv` as needed to inform the
/// approximation.
diff --git a/clang/include/clang/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.h b/clang/include/clang/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.h
index 9ed3e25d63327..88a33d19f7d8f 100644
--- a/clang/include/clang/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.h
+++ b/clang/include/clang/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.h
@@ -72,7 +72,7 @@ class TypeErasedDataflowAnalysis : public Environment::ValueModel {
/// Joins two type-erased lattice elements by computing their least upper
/// bound. Places the join result in the left element and returns an effect
/// indicating whether any changes were made to it.
- virtual LatticeJoinEffect joinTypeErased(TypeErasedLattice &,
+ virtual TypeErasedLattice joinTypeErased(const TypeErasedLattice &,
const TypeErasedLattice &) = 0;
/// Chooses a lattice element that approximates the current element at a
diff --git a/clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp b/clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp
index 3296fee39dd13..44801a41c10fa 100644
--- a/clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp
+++ b/clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp
@@ -526,18 +526,18 @@ LatticeJoinEffect Environment::widen(const Environment &PrevEnv,
return Effect;
}
-Environment Environment::join(const Environment &Other,
- Environment::ValueModel &Model) const {
- assert(DACtx == Other.DACtx);
- assert(ThisPointeeLoc == Other.ThisPointeeLoc);
- assert(CallStack == Other.CallStack);
+Environment Environment::join(const Environment &EnvA, const Environment &EnvB,
+ Environment::ValueModel &Model) {
+ assert(EnvA.DACtx == EnvB.DACtx);
+ assert(EnvA.ThisPointeeLoc == EnvB.ThisPointeeLoc);
+ assert(EnvA.CallStack == EnvB.CallStack);
- Environment JoinedEnv(*DACtx);
+ Environment JoinedEnv(*EnvA.DACtx);
- JoinedEnv.CallStack = CallStack;
- JoinedEnv.ThisPointeeLoc = ThisPointeeLoc;
+ JoinedEnv.CallStack = EnvA.CallStack;
+ JoinedEnv.ThisPointeeLoc = EnvA.ThisPointeeLoc;
- if (ReturnVal == nullptr || Other.ReturnVal == nullptr) {
+ if (EnvA.ReturnVal == nullptr || EnvB.ReturnVal == nullptr) {
// `ReturnVal` might not always get set -- for example if we have a return
// statement of the form `return some_other_func()` and we decide not to
// analyze `some_other_func()`.
@@ -546,22 +546,22 @@ Environment Environment::join(const Environment &Other,
// it might not be the correct one.
// This occurs for example in the test `ContextSensitiveMutualRecursion`.
JoinedEnv.ReturnVal = nullptr;
- } else if (areEquivalentValues(*ReturnVal, *Other.ReturnVal)) {
- JoinedEnv.ReturnVal = ReturnVal;
+ } else if (areEquivalentValues(*EnvA.ReturnVal, *EnvB.ReturnVal)) {
+ JoinedEnv.ReturnVal = EnvA.ReturnVal;
} else {
- assert(!CallStack.empty());
+ assert(!EnvA.CallStack.empty());
// FIXME: Make `CallStack` a vector of `FunctionDecl` so we don't need this
// cast.
- auto *Func = dyn_cast<FunctionDecl>(CallStack.back());
+ auto *Func = dyn_cast<FunctionDecl>(EnvA.CallStack.back());
assert(Func != nullptr);
if (Value *MergedVal =
- mergeDistinctValues(Func->getReturnType(), *ReturnVal, *this,
- *Other.ReturnVal, Other, JoinedEnv, Model))
+ mergeDistinctValues(Func->getReturnType(), *EnvA.ReturnVal, EnvA,
+ *EnvB.ReturnVal, EnvB, JoinedEnv, Model))
JoinedEnv.ReturnVal = MergedVal;
}
- if (ReturnLoc == Other.ReturnLoc)
- JoinedEnv.ReturnLoc = ReturnLoc;
+ if (EnvA.ReturnLoc == EnvB.ReturnLoc)
+ JoinedEnv.ReturnLoc = EnvA.ReturnLoc;
else
JoinedEnv.ReturnLoc = nullptr;
@@ -569,27 +569,27 @@ Environment Environment::join(const Environment &Other,
// lifetime ends, add an assertion that there aren't any entries in
// `DeclToLoc` and `Other.DeclToLoc` that map the same declaration to
//
diff erent storage locations.
- JoinedEnv.DeclToLoc = intersectDenseMaps(DeclToLoc, Other.DeclToLoc);
+ JoinedEnv.DeclToLoc = intersectDenseMaps(EnvA.DeclToLoc, EnvB.DeclToLoc);
- JoinedEnv.ExprToLoc = intersectDenseMaps(ExprToLoc, Other.ExprToLoc);
+ JoinedEnv.ExprToLoc = intersectDenseMaps(EnvA.ExprToLoc, EnvB.ExprToLoc);
JoinedEnv.MemberLocToStruct =
- intersectDenseMaps(MemberLocToStruct, Other.MemberLocToStruct);
+ intersectDenseMaps(EnvA.MemberLocToStruct, EnvB.MemberLocToStruct);
// FIXME: update join to detect backedges and simplify the flow condition
// accordingly.
- JoinedEnv.FlowConditionToken = &DACtx->joinFlowConditions(
- *FlowConditionToken, *Other.FlowConditionToken);
+ JoinedEnv.FlowConditionToken = &EnvA.DACtx->joinFlowConditions(
+ *EnvA.FlowConditionToken, *EnvB.FlowConditionToken);
- for (auto &Entry : LocToVal) {
+ for (auto &Entry : EnvA.LocToVal) {
const StorageLocation *Loc = Entry.first;
assert(Loc != nullptr);
Value *Val = Entry.second;
assert(Val != nullptr);
- auto It = Other.LocToVal.find(Loc);
- if (It == Other.LocToVal.end())
+ auto It = EnvB.LocToVal.find(Loc);
+ if (It == EnvB.LocToVal.end())
continue;
assert(It->second != nullptr);
@@ -598,9 +598,8 @@ Environment Environment::join(const Environment &Other,
continue;
}
- if (Value *MergedVal =
- mergeDistinctValues(Loc->getType(), *Val, *this, *It->second, Other,
- JoinedEnv, Model)) {
+ if (Value *MergedVal = mergeDistinctValues(
+ Loc->getType(), *Val, EnvA, *It->second, EnvB, JoinedEnv, Model)) {
JoinedEnv.LocToVal.insert({Loc, MergedVal});
}
}
diff --git a/clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp b/clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp
index 880ad6fa8bcf6..16f8ab9683d56 100644
--- a/clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp
+++ b/clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp
@@ -227,50 +227,39 @@ class PrettyStackTraceCFGElement : public llvm::PrettyStackTraceEntry {
// Avoids unneccesary copies of the environment.
class JoinedStateBuilder {
AnalysisContext &AC;
- std::optional<TypeErasedDataflowAnalysisState> OwnedState;
- // Points either to OwnedState, an external Environment, or nothing.
- const TypeErasedDataflowAnalysisState *CurrentState = nullptr;
+ std::vector<const TypeErasedDataflowAnalysisState *> All;
+ std::deque<TypeErasedDataflowAnalysisState> Owned;
+
+ TypeErasedDataflowAnalysisState
+ join(const TypeErasedDataflowAnalysisState &L,
+ const TypeErasedDataflowAnalysisState &R) {
+ return {AC.Analysis.joinTypeErased(L.Lattice, R.Lattice),
+ Environment::join(L.Env, R.Env, AC.Analysis)};
+ }
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);
- }
+ Owned.push_back(std::move(State));
+ All.push_back(&Owned.back());
}
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);
- }
+ All.push_back(&State);
}
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);
+ if (All.empty())
+ // 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.
+ return {AC.Analysis.typeErasedInitialElement(), AC.InitEnv.fork()};
+ if (All.size() == 1)
+ return Owned.empty() ? All.front()->fork() : std::move(Owned.front());
+
+ auto Result = join(*All[0], *All[1]);
+ for (unsigned I = 2; I < All.size(); ++I)
+ Result = join(Result, *All[I]);
+ return Result;
}
};
More information about the cfe-commits
mailing list