[PATCH] D82122: [analyzer][Liveness][RFC][NFC] Propose to turn statement liveness into expression liveness

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus added a comment.

This was accidentally committed as a part of 
rGb6cbe6cb0399d4671e5384dcc326af56bc6bd122 
.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82122



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

[PATCH] D82122: [analyzer][Liveness][RFC][NFC] Propose to turn statement liveness into expression liveness

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus abandoned this revision.
Szelethus added a comment.

This revision has done its job.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82122



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

[PATCH] D82122: [analyzer][Liveness][RFC][NFC] Propose to turn statement liveness into expression liveness

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus added a comment.

Cheers! I'll leave this here for conversation's sake, and start working on 
refactoring LivenessAnalysis.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82122



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

[PATCH] D82122: [analyzer][Liveness][RFC][NFC] Propose to turn statement liveness into expression liveness

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus marked an inline comment as done.
Szelethus added inline comments.



Comment at: clang/lib/StaticAnalyzer/Core/Environment.cpp:193
+
+assert((isa(BlkExpr.getStmt()) || !IsBlkExprLive) &&
+   "Only Exprs can be live, LivenessAnalysis argues about the liveness 
"

>>! In D82122#2103638, @NoQ wrote:
> I think you're right, this entire thing only makes sense for expressions. I 
> think `Environment` occasionally carries values of `ReturnStmt`s (by which it 
> means values of their sub-expressions) but even then liveness analysis 
> doesn't need to be aware of that flex.
> 
>> no lit tests crashed on the added asserts
> 
>> the only non-expression statement it queries are `ObjCForCollectionStmt`s
> 
> Wait, how did you figure that out if not through crashing tests?

```lang=c++
if (IsBlkExprLive && !isa(BlkExpr.getStmt()) {
  BlkExpr.getStmt()->dump();
  llvm_unreachable("Gotcha!");
}



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82122



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

[PATCH] D82122: [analyzer][Liveness][RFC][NFC] Propose to turn statement liveness into expression liveness

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a comment.

I think you're right, this entire thing only makes sense for expressions. I 
think `Environment` occasionally carries values of `ReturnStmt`s (by which it 
means values of their sub-expressions) but even then liveness analysis doesn't 
need to be aware of that flex.

> no lit tests crashed on the added asserts



> the only non-expression statement it queries are `ObjCForCollectionStmt`s

Wait, how did you figure that out if not through crashing tests?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82122



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

[PATCH] D82122: [analyzer][Liveness][RFC][NFC] Propose to turn statement liveness into expression liveness

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus created this revision.
Szelethus added reviewers: xazax.hun, NoQ, vsavchenko, balazske, martong, 
baloghadamsoftware, dcoughlin.
Szelethus added a project: clang.
Herald added subscribers: cfe-commits, ASDenysPetrov, steakhal, Charusso, 
gamesh411, dkrupp, donat.nagy, mikhail.ramalho, a.sidorin, rnkovacs, szepet, 
whisperity.

I had a bit of fun with the tags.

Liveness analysis calculates the set of live/dead values in the program. 
However, statements don't always have a value associated with them, expressions 
do. For this reason, I found it puzzling why LivenessAnalysis work with "live 
statements", or answers questions such as "is this statement live". After a bit 
of digging, I found that the only client of statement liveness is 
`EnvironmentManager::removeDeadBindings` (the one modified in this patch), and 
the only non-expression statement it queries are `ObjCForCollectionStmt`s, but 
no lit tests crashed on the added asserts.

This patch isn't necesserily intended to be committed -- it more of a request 
for comments whether it would be safe to change statement liveness to strictly 
expression liveness. It would be great, because my ultimate goal is to merge 
the implementation of UninitializedVariable and LivenessAnalaysis, and add 
Reaching Definitions under the same hood, but the statement liveness was very 
much out of place and makes the merge very awkward, if even possible.

I have a number of changes planned, but I didn't want to go too far ahead of 
myself, in case I missed something fundamental that justifies statement 
liveness as it is.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D82122

Files:
  clang/lib/StaticAnalyzer/Core/Environment.cpp


Index: clang/lib/StaticAnalyzer/Core/Environment.cpp
===
--- clang/lib/StaticAnalyzer/Core/Environment.cpp
+++ clang/lib/StaticAnalyzer/Core/Environment.cpp
@@ -183,12 +183,18 @@
  F.getTreeFactory());
 
   // Iterate over the block-expr bindings.
-  for (Environment::iterator I = Env.begin(), E = Env.end();
-   I != E; ++I) {
+  for (Environment::iterator I = Env.begin(), E = Env.end(); I != E; ++I) {
 const EnvironmentEntry  = I.getKey();
 const SVal  = I.getData();
 
-if (SymReaper.isLive(BlkExpr.getStmt(), BlkExpr.getLocationContext())) {
+const bool IsBlkExprLive =
+SymReaper.isLive(BlkExpr.getStmt(), BlkExpr.getLocationContext());
+
+assert((isa(BlkExpr.getStmt()) || !IsBlkExprLive) &&
+   "Only Exprs can be live, LivenessAnalysis argues about the liveness 
"
+   "of *values*!");
+
+if (IsBlkExprLive) {
   // Copy the binding to the new map.
   EBMapRef = EBMapRef.add(BlkExpr, X);
 


Index: clang/lib/StaticAnalyzer/Core/Environment.cpp
===
--- clang/lib/StaticAnalyzer/Core/Environment.cpp
+++ clang/lib/StaticAnalyzer/Core/Environment.cpp
@@ -183,12 +183,18 @@
  F.getTreeFactory());
 
   // Iterate over the block-expr bindings.
-  for (Environment::iterator I = Env.begin(), E = Env.end();
-   I != E; ++I) {
+  for (Environment::iterator I = Env.begin(), E = Env.end(); I != E; ++I) {
 const EnvironmentEntry  = I.getKey();
 const SVal  = I.getData();
 
-if (SymReaper.isLive(BlkExpr.getStmt(), BlkExpr.getLocationContext())) {
+const bool IsBlkExprLive =
+SymReaper.isLive(BlkExpr.getStmt(), BlkExpr.getLocationContext());
+
+assert((isa(BlkExpr.getStmt()) || !IsBlkExprLive) &&
+   "Only Exprs can be live, LivenessAnalysis argues about the liveness "
+   "of *values*!");
+
+if (IsBlkExprLive) {
   // Copy the binding to the new map.
   EBMapRef = EBMapRef.add(BlkExpr, X);
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits