[PATCH] D158977: [clang][dataflow] Don't associate prvalue expressions with storage locations.

[Martin Böhme via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG330d5bcbf610: [clang][dataflow] Don't associate prvalue 
expressions with storage locations. (authored by mboehme).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D158977

Files:
  clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h
  clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp
  clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
  clang/lib/Analysis/FlowSensitive/Transfer.cpp

Index: clang/lib/Analysis/FlowSensitive/Transfer.cpp
===
--- clang/lib/Analysis/FlowSensitive/Transfer.cpp
+++ clang/lib/Analysis/FlowSensitive/Transfer.cpp
@@ -488,6 +488,7 @@
 // we've got a record type.
 if (S->getType()->isRecordType()) {
   auto &InitialVal = *cast(Env.createValue(S->getType()));
+  Env.setValue(*S, InitialVal);
   copyRecord(InitialVal.getLoc(), Env.getResultObjectLocation(*S), Env);
 }
 
Index: clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
===
--- clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
+++ clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
@@ -458,8 +458,9 @@
 void transferMakeOptionalCall(const CallExpr *E,
   const MatchFinder::MatchResult &,
   LatticeTransferState &State) {
-  createOptionalValue(State.Env.getResultObjectLocation(*E),
-  State.Env.getBoolLiteralValue(true), State.Env);
+  State.Env.setValue(
+  *E, createOptionalValue(State.Env.getResultObjectLocation(*E),
+  State.Env.getBoolLiteralValue(true), State.Env));
 }
 
 void transferOptionalHasValueCall(const CXXMemberCallExpr *CallExpr,
@@ -543,7 +544,10 @@
 }
   }
 
-  createOptionalValue(*Loc, State.Env.makeAtomicBoolValue(), State.Env);
+  RecordValue &Val =
+  createOptionalValue(*Loc, State.Env.makeAtomicBoolValue(), State.Env);
+  if (E->isPRValue())
+State.Env.setValue(*E, Val);
 }
 
 void constructOptionalValue(const Expr &E, Environment &Env,
Index: clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp
===
--- clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp
+++ clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp
@@ -171,6 +171,105 @@
   return Current;
 }
 
+// Returns whether the values in `Map1` and `Map2` compare equal for those
+// keys that `Map1` and `Map2` have in common.
+template 
+bool compareKeyToValueMaps(const llvm::MapVector &Map1,
+   const llvm::MapVector &Map2,
+   const Environment &Env1, const Environment &Env2,
+   Environment::ValueModel &Model) {
+  for (auto &Entry : Map1) {
+Key K = Entry.first;
+assert(K != nullptr);
+
+Value *Val = Entry.second;
+assert(Val != nullptr);
+
+auto It = Map2.find(K);
+if (It == Map2.end())
+  continue;
+assert(It->second != nullptr);
+
+if (!areEquivalentValues(*Val, *It->second) &&
+!compareDistinctValues(K->getType(), *Val, Env1, *It->second, Env2,
+   Model))
+  return false;
+  }
+
+  return true;
+}
+
+// Perform a join on either `LocToVal` or `ExprToVal`. `Key` must be either
+// `const StorageLocation *` or `const Expr *`.
+template 
+llvm::MapVector
+joinKeyToValueMap(const llvm::MapVector &Map1,
+  const llvm::MapVector &Map2,
+  const Environment &Env1, const Environment &Env2,
+  Environment &JoinedEnv, Environment::ValueModel &Model) {
+  llvm::MapVector MergedMap;
+  for (auto &Entry : Map1) {
+Key K = Entry.first;
+assert(K != nullptr);
+
+Value *Val = Entry.second;
+assert(Val != nullptr);
+
+auto It = Map2.find(K);
+if (It == Map2.end())
+  continue;
+assert(It->second != nullptr);
+
+if (areEquivalentValues(*Val, *It->second)) {
+  MergedMap.insert({K, Val});
+  continue;
+}
+
+if (Value *MergedVal = mergeDistinctValues(
+K->getType(), *Val, Env1, *It->second, Env2, JoinedEnv, Model)) {
+  MergedMap.insert({K, MergedVal});
+}
+  }
+
+  return MergedMap;
+}
+
+// Perform widening on either `LocToVal` or `ExprToVal`. `Key` must be either
+// `const StorageLocation *` or `const Expr *`.
+template 
+llvm::MapVector
+widenKeyToValueMap(const llvm::MapVector &CurMap,
+   const llvm::MapVector &PrevMap,
+   Environment &CurEnv, const Environment &PrevEnv,
+   Environment::ValueModel &Model, LatticeJoinEffect &Effect) {
+  llvm::MapVector WidenedMap;
+  for (auto &Entry : CurMap) {
+Key K = Entry.first;
+   

[PATCH] D158977: [clang][dataflow] Don't associate prvalue expressions with storage locations.

[Martin Böhme via Phabricator via cfe-commits]

mboehme added a comment.

In D158977#4623402 , @xazax.hun wrote:

> Thanks! Sometimes I am wondering whether we actually need a full map for 
> PRValues. E.g., once we processed a `MaterializeTemporaryExpr`, we now have a 
> location for the value, and it feels like we represent the same thing twice, 
> once in `ExprToLoc + LocToVal` and once in `ExprToVal`. It is probably not 
> too bad and might be extra work to clean this up.

Yes, I think it's probably not worth it. (And note that the `Expr`s in question 
are different: In the `ExprToVal`, we map the prvalue expression to a value, 
whereas in `ExprToLoc`, we map the `MaterializeTemporaryExpr` to a location.)

I'd say this is just one example of the more general case when a prvalue is 
consumed by some other expression. For example, when two prvalue integer 
operands are consumed by a `+` `BinaryOperator`, we could also say that the 
entries in `ExprToVal` for the operands are now superfluous and could be 
removed -- but we still keep them around. They might be superfluous, but there 
aren't typically enough of them to hurt performance (I think?). And they may be 
useful for debugging.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D158977

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

[PATCH] D158977: [clang][dataflow] Don't associate prvalue expressions with storage locations.

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun accepted this revision.
xazax.hun added a comment.
This revision is now accepted and ready to land.

Thanks! Sometimes I am wondering whether we actually need a full map for 
PRValues. E.g., once we processed a `MaterializeTemporaryExpr`, we now have a 
location for the value, and it feels like we represent the same thing twice, 
once in `ExprToLoc + LocToVal` and once in `ExprToVal`. It is probably not too 
bad and might be extra work to clean this up.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D158977

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

[PATCH] D158977: [clang][dataflow] Don't associate prvalue expressions with storage locations.

[Martin Böhme via Phabricator via cfe-commits]

mboehme created this revision.
Herald added subscribers: martong, xazax.hun.
Herald added a reviewer: NoQ.
Herald added a project: All.
mboehme requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

Instead, map prvalue expressions directly to values in a newly introduced map
`Environment::ExprToVal`.

This change introduces an additional member variable in `Environment` but is
an overall win:

- It is more conceptually correctly, since prvalues don't have storage 
locations.

- It eliminates complexity from `Environment::setValue(const Expr &E, Value 
&Val)`.

- It reduces the amount of data stored in `Environment`: A prvalue now has a 
single entry in `ExprToVal` instead of one in `ExprToLoc` and one in `LocToVal`.

- Not allocating `StorageLocation`s for prvalues additionally reduces memory 
usage.

This patch is the last step in the migration to strict handling of value
categories (see https://discourse.llvm.org/t/70086 for details). The changes
here are almost entirely internal to `Environment`.

The only externally observable change is that when associating a `RecordValue`
with the location returned by `Environment::getResultObjectLocation()` for a
given expression, callers additionally need to associate the `RecordValue` with
the expression themselves.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D158977

Files:
  clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h
  clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp
  clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
  clang/lib/Analysis/FlowSensitive/Transfer.cpp

Index: clang/lib/Analysis/FlowSensitive/Transfer.cpp
===
--- clang/lib/Analysis/FlowSensitive/Transfer.cpp
+++ clang/lib/Analysis/FlowSensitive/Transfer.cpp
@@ -488,6 +488,7 @@
 // we've got a record type.
 if (S->getType()->isRecordType()) {
   auto &InitialVal = *cast(Env.createValue(S->getType()));
+  Env.setValue(*S, InitialVal);
   copyRecord(InitialVal.getLoc(), Env.getResultObjectLocation(*S), Env);
 }
 
Index: clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
===
--- clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
+++ clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
@@ -458,8 +458,9 @@
 void transferMakeOptionalCall(const CallExpr *E,
   const MatchFinder::MatchResult &,
   LatticeTransferState &State) {
-  createOptionalValue(State.Env.getResultObjectLocation(*E),
-  State.Env.getBoolLiteralValue(true), State.Env);
+  State.Env.setValue(
+  *E, createOptionalValue(State.Env.getResultObjectLocation(*E),
+  State.Env.getBoolLiteralValue(true), State.Env));
 }
 
 void transferOptionalHasValueCall(const CXXMemberCallExpr *CallExpr,
@@ -543,7 +544,10 @@
 }
   }
 
-  createOptionalValue(*Loc, State.Env.makeAtomicBoolValue(), State.Env);
+  RecordValue &Val =
+  createOptionalValue(*Loc, State.Env.makeAtomicBoolValue(), State.Env);
+  if (E->isPRValue())
+State.Env.setValue(*E, Val);
 }
 
 void constructOptionalValue(const Expr &E, Environment &Env,
Index: clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp
===
--- clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp
+++ clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp
@@ -171,6 +171,105 @@
   return Current;
 }
 
+// Returns whether the values in `Map1` and `Map2` compare equal for those
+// keys that `Map1` and `Map2` have in common.
+template 
+bool compareKeyToValueMaps(const llvm::MapVector &Map1,
+   const llvm::MapVector &Map2,
+   const Environment &Env1, const Environment &Env2,
+   Environment::ValueModel &Model) {
+  for (auto &Entry : Map1) {
+Key K = Entry.first;
+assert(K != nullptr);
+
+Value *Val = Entry.second;
+assert(Val != nullptr);
+
+auto It = Map2.find(K);
+if (It == Map2.end())
+  continue;
+assert(It->second != nullptr);
+
+if (!areEquivalentValues(*Val, *It->second) &&
+!compareDistinctValues(K->getType(), *Val, Env1, *It->second, Env2,
+   Model))
+  return false;
+  }
+
+  return true;
+}
+
+// Perform a join on either `LocToVal` or `ExprToVal`. `Key` must be either
+// `const StorageLocation *` or `const Expr *`.
+template 
+llvm::MapVector
+joinKeyToValueMap(const llvm::MapVector &Map1,
+  const llvm::MapVector &Map2,
+  const Environment &Env1, const Environment &Env2,
+  Environment &JoinedEnv, Environment::ValueModel &Model) {
+  llvm::MapVector MergedMap;
+  for (auto &Entry : Map