[PATCH] D58035: [clang/DIVar] Emit flag for params that have unchanged values

[Djordje Todorovic via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL364424: [clang/DIVar] Emit the flag for params that have 
unmodified value (authored by djtodoro, committed by ).
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D58035?vs=201505=206651#toc

Repository:
  rL LLVM

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

https://reviews.llvm.org/D58035

Files:
  cfe/trunk/lib/CodeGen/CGDebugInfo.cpp
  cfe/trunk/lib/CodeGen/CGDebugInfo.h
  cfe/trunk/test/CodeGen/debug-info-param-modification.c

Index: cfe/trunk/test/CodeGen/debug-info-param-modification.c
===
--- cfe/trunk/test/CodeGen/debug-info-param-modification.c
+++ cfe/trunk/test/CodeGen/debug-info-param-modification.c
@@ -0,0 +1,12 @@
+// RUN: %clang -Xclang -femit-debug-entry-values -g -O2 -S -target x86_64-none-linux-gnu -emit-llvm %s -o - | FileCheck %s -check-prefix=CHECK-ENTRY-VAL-OPT
+// CHECK-ENTRY-VAL-OPT: !DILocalVariable(name: "a", arg: 1, scope: {{.*}}, file: {{.*}}, line: {{.*}}, type: {{.*}})
+// CHECK-ENTRY-VAL-OPT: !DILocalVariable(name: "b", arg: 2, scope: {{.*}}, file: {{.*}}, line: {{.*}}, type: {{.*}}, flags: DIFlagArgumentNotModified)
+//
+// RUN: %clang -g -O2 -target x86_64-none-linux-gnu -S -emit-llvm %s -o - | FileCheck %s
+// CHECK-NOT: !DILocalVariable(name: "b", arg: 2, scope: {{.*}}, file: {{.*}}, line: {{.*}}, type: {{.*}}, flags: DIFlagArgumentNotModified)
+//
+
+int fn2 (int a, int b) {
+  ++a;
+  return b;
+}
Index: cfe/trunk/lib/CodeGen/CGDebugInfo.h
===
--- cfe/trunk/lib/CodeGen/CGDebugInfo.h
+++ cfe/trunk/lib/CodeGen/CGDebugInfo.h
@@ -134,6 +134,10 @@
 
   llvm::DenseMap DIFileCache;
   llvm::DenseMap SPCache;
+  /// Cache function definitions relevant to use for parameters mutation
+  /// analysis.
+  llvm::DenseMap SPDefCache;
+  llvm::DenseMap ParamCache;
   /// Cache declarations relevant to DW_TAG_imported_declarations (C++
   /// using declarations) that aren't covered by other more specific caches.
   llvm::DenseMap DeclCache;
Index: cfe/trunk/lib/CodeGen/CGDebugInfo.cpp
===
--- cfe/trunk/lib/CodeGen/CGDebugInfo.cpp
+++ cfe/trunk/lib/CodeGen/CGDebugInfo.cpp
@@ -18,6 +18,7 @@
 #include "CodeGenFunction.h"
 #include "CodeGenModule.h"
 #include "ConstantEmitter.h"
+#include "clang/Analysis/Analyses/ExprMutationAnalyzer.h"
 #include "clang/AST/ASTContext.h"
 #include "clang/AST/DeclFriend.h"
 #include "clang/AST/DeclObjC.h"
@@ -3588,6 +3589,12 @@
   if (HasDecl && isa(D))
 DeclCache[D->getCanonicalDecl()].reset(SP);
 
+  // We use the SPDefCache only in the case when the debug entry values option
+  // is set, in order to speed up parameters modification analysis.
+  if (CGM.getCodeGenOpts().EnableDebugEntryValues && HasDecl &&
+  isa(D))
+SPDefCache[cast(D)].reset(SP);
+
   if (CGM.getCodeGenOpts().DwarfVersion >= 5) {
 // Starting with DWARF V5 method declarations are emitted as children of
 // the interface type.
@@ -3964,6 +3971,11 @@
  llvm::DebugLoc::get(Line, Column, Scope, CurInlinedAt),
  Builder.GetInsertBlock());
 
+  if (CGM.getCodeGenOpts().EnableDebugEntryValues && ArgNo) {
+if (auto *PD = dyn_cast(VD))
+  ParamCache[PD].reset(D);
+  }
+
   return D;
 }
 
@@ -4555,6 +4567,29 @@
   TheCU->setDWOId(Signature);
 }
 
+/// Analyzes each function parameter to determine whether it is constant
+/// throughout the function body.
+static void analyzeParametersModification(
+ASTContext ,
+llvm::DenseMap ,
+llvm::DenseMap ) {
+  for (auto  : SPDefCache) {
+auto *FD = SP.first;
+assert(FD->hasBody() && "Functions must have body here");
+const Stmt *FuncBody = (*FD).getBody();
+for (auto Parm : FD->parameters()) {
+  ExprMutationAnalyzer FuncAnalyzer(*FuncBody, Ctx);
+  if (FuncAnalyzer.isMutated(Parm))
+continue;
+
+  auto I = ParamCache.find(Parm);
+  assert(I != ParamCache.end() && "Parameters should be already cached");
+  auto *DIParm = cast(I->second);
+  DIParm->setIsNotModified();
+}
+  }
+}
+
 void CGDebugInfo::finalize() {
   // Creating types might create further types - invalidating the current
   // element and the size(), so don't cache/reference them.
@@ -4627,6 +4662,10 @@
 if (auto MD = TypeCache[RT])
   DBuilder.retainType(cast(MD));
 
+  if (CGM.getCodeGenOpts().EnableDebugEntryValues)
+// This will be used to emit debug entry values.
+analyzeParametersModification(CGM.getContext(), SPDefCache, ParamCache);
+
   DBuilder.finalize();
 }
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D58035: [clang/DIVar] Emit flag for params that have unchanged values

[Djordje Todorovic via Phabricator via cfe-commits]

djtodoro updated this revision to Diff 201505.
djtodoro added a comment.

-Rebase
-Add a test


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

https://reviews.llvm.org/D58035

Files:
  lib/CodeGen/CGDebugInfo.cpp
  lib/CodeGen/CGDebugInfo.h
  test/CodeGen/debug-info-param-modification.c

Index: test/CodeGen/debug-info-param-modification.c
===
--- /dev/null
+++ test/CodeGen/debug-info-param-modification.c
@@ -0,0 +1,12 @@
+// RUN: %clang -Xclang -femit-debug-entry-values -g -O2 -S -emit-llvm %s -o - | FileCheck %s -check-prefix=CHECK-ENTRY-VAL-OPT
+// CHECK-ENTRY-VAL-OPT: !DILocalVariable(name: "a", arg: 1, scope: {{.*}}, file: {{.*}}, line: {{.*}}, type: {{.*}})
+// CHECK-ENTRY-VAL-OPT: !DILocalVariable(name: "b", arg: 2, scope: {{.*}}, file: {{.*}}, line: {{.*}}, type: {{.*}}, flags: DIFlagArgumentNotModified)
+//
+// RUN: %clang -g -O2 -S -emit-llvm %s -o - | FileCheck %s
+// CHECK-NOT: !DILocalVariable(name: "b", arg: 2, scope: {{.*}}, file: {{.*}}, line: {{.*}}, type: {{.*}}, flags: DIFlagArgumentNotModified)
+//
+
+int fn2 (int a, int b) {
+  ++a;
+  return b;
+}
Index: lib/CodeGen/CGDebugInfo.h
===
--- lib/CodeGen/CGDebugInfo.h
+++ lib/CodeGen/CGDebugInfo.h
@@ -134,6 +134,10 @@
 
   llvm::DenseMap DIFileCache;
   llvm::DenseMap SPCache;
+  /// Cache function definitions relevant to use for parameters mutation
+  /// analysis.
+  llvm::DenseMap SPDefCache;
+  llvm::DenseMap ParamCache;
   /// Cache declarations relevant to DW_TAG_imported_declarations (C++
   /// using declarations) that aren't covered by other more specific caches.
   llvm::DenseMap DeclCache;
Index: lib/CodeGen/CGDebugInfo.cpp
===
--- lib/CodeGen/CGDebugInfo.cpp
+++ lib/CodeGen/CGDebugInfo.cpp
@@ -18,6 +18,7 @@
 #include "CodeGenFunction.h"
 #include "CodeGenModule.h"
 #include "ConstantEmitter.h"
+#include "clang/Analysis/Analyses/ExprMutationAnalyzer.h"
 #include "clang/AST/ASTContext.h"
 #include "clang/AST/DeclFriend.h"
 #include "clang/AST/DeclObjC.h"
@@ -3577,6 +3578,12 @@
   if (HasDecl && isa(D))
 DeclCache[D->getCanonicalDecl()].reset(SP);
 
+  // We use the SPDefCache only in the case when the debug entry values option
+  // is set, in order to speed up parameters modification analysis.
+  if (CGM.getCodeGenOpts().EnableDebugEntryValues && HasDecl &&
+  isa(D))
+SPDefCache[cast(D)].reset(SP);
+
   if (CGM.getCodeGenOpts().DwarfVersion >= 5) {
 // Starting with DWARF V5 method declarations are emitted as children of
 // the interface type.
@@ -3942,6 +3949,11 @@
  llvm::DebugLoc::get(Line, Column, Scope, CurInlinedAt),
  Builder.GetInsertBlock());
 
+  if (CGM.getCodeGenOpts().EnableDebugEntryValues && ArgNo) {
+if (auto *PD = dyn_cast(VD))
+  ParamCache[PD].reset(D);
+  }
+
   return D;
 }
 
@@ -4515,6 +4527,29 @@
   TheCU->setDWOId(Signature);
 }
 
+/// Analyzes each function parameter to determine whether it is constant
+/// throughout the function body.
+static void analyzeParametersModification(
+ASTContext ,
+llvm::DenseMap ,
+llvm::DenseMap ) {
+  for (auto  : SPDefCache) {
+auto *FD = SP.first;
+assert(FD->hasBody() && "Functions must have body here");
+const Stmt *FuncBody = (*FD).getBody();
+for (auto Parm : FD->parameters()) {
+  ExprMutationAnalyzer FuncAnalyzer(*FuncBody, Ctx);
+  if (FuncAnalyzer.isMutated(Parm))
+continue;
+
+  auto I = ParamCache.find(Parm);
+  assert(I != ParamCache.end() && "Parameters should be already cached");
+  auto *DIParm = cast(I->second);
+  DIParm->setIsNotModified();
+}
+  }
+}
+
 void CGDebugInfo::finalize() {
   // Creating types might create further types - invalidating the current
   // element and the size(), so don't cache/reference them.
@@ -4587,6 +4622,10 @@
 if (auto MD = TypeCache[RT])
   DBuilder.retainType(cast(MD));
 
+  if (CGM.getCodeGenOpts().EnableDebugEntryValues)
+// This will be used to emit debug entry values.
+analyzeParametersModification(CGM.getContext(), SPDefCache, ParamCache);
+
   DBuilder.finalize();
 }
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D58035: [clang/DIVar] Emit flag for params that have unchanged values

[Djordje Todorovic via Phabricator via cfe-commits]

djtodoro updated this revision to Diff 200950.
djtodoro added a comment.

-Add `SPDefCache` to speed up the process
-Add additional assertions that will improve quality of the code


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

https://reviews.llvm.org/D58035

Files:
  lib/CodeGen/CGDebugInfo.cpp
  lib/CodeGen/CGDebugInfo.h


Index: lib/CodeGen/CGDebugInfo.h
===
--- lib/CodeGen/CGDebugInfo.h
+++ lib/CodeGen/CGDebugInfo.h
@@ -133,6 +133,10 @@
 
   llvm::DenseMap DIFileCache;
   llvm::DenseMap SPCache;
+  /// Cache function definitions relevant to use for parameters mutation
+  /// analysis.
+  llvm::DenseMap SPDefCache;
+  llvm::DenseMap ParamCache;
   /// Cache declarations relevant to DW_TAG_imported_declarations (C++
   /// using declarations) that aren't covered by other more specific caches.
   llvm::DenseMap DeclCache;
Index: lib/CodeGen/CGDebugInfo.cpp
===
--- lib/CodeGen/CGDebugInfo.cpp
+++ lib/CodeGen/CGDebugInfo.cpp
@@ -18,6 +18,7 @@
 #include "CodeGenFunction.h"
 #include "CodeGenModule.h"
 #include "ConstantEmitter.h"
+#include "clang/Analysis/Analyses/ExprMutationAnalyzer.h"
 #include "clang/AST/ASTContext.h"
 #include "clang/AST/DeclFriend.h"
 #include "clang/AST/DeclObjC.h"
@@ -3515,6 +3516,12 @@
   if (HasDecl && isa(D))
 DeclCache[D->getCanonicalDecl()].reset(SP);
 
+  // We use the SPDefCache only in the case when the debug entry values option
+  // is set, in order to speed up parameters modification analysis.
+  if (CGM.getCodeGenOpts().EnableDebugEntryValues && HasDecl &&
+  isa(D))
+SPDefCache[cast(D)].reset(SP);
+
   if (CGM.getCodeGenOpts().DwarfVersion >= 5) {
 // Starting with DWARF V5 method declarations are emitted as children of
 // the interface type.
@@ -3880,6 +3887,11 @@
  llvm::DebugLoc::get(Line, Column, Scope, 
CurInlinedAt),
  Builder.GetInsertBlock());
 
+  if (CGM.getCodeGenOpts().EnableDebugEntryValues && ArgNo) {
+if (auto *PD = dyn_cast(VD))
+  ParamCache[PD].reset(D);
+  }
+
   return D;
 }
 
@@ -4454,6 +4466,29 @@
   TheCU->setDWOId(Signature);
 }
 
+/// Analyzes each function parameter to determine whether it is constant
+/// throughout the function body.
+static void analyzeParametersModification(
+ASTContext ,
+llvm::DenseMap ,
+llvm::DenseMap ) {
+  for (auto  : SPDefCache) {
+auto *FD = SP.first;
+assert(FD->hasBody() && "Functions must have body here");
+const Stmt *FuncBody = (*FD).getBody();
+for (auto Parm : FD->parameters()) {
+  ExprMutationAnalyzer FuncAnalyzer(*FuncBody, Ctx);
+  if (FuncAnalyzer.isMutated(Parm))
+continue;
+
+  auto I = ParamCache.find(Parm);
+  assert(I != ParamCache.end() && "Parameters should be already cached");
+  auto *DIParm = cast(I->second);
+  DIParm->setIsNotModified();
+}
+  }
+}
+
 void CGDebugInfo::finalize() {
   // Creating types might create further types - invalidating the current
   // element and the size(), so don't cache/reference them.
@@ -4526,6 +4561,10 @@
 if (auto MD = TypeCache[RT])
   DBuilder.retainType(cast(MD));
 
+  if (CGM.getCodeGenOpts().EnableDebugEntryValues)
+// This will be used to emit debug entry values.
+analyzeParametersModification(CGM.getContext(), SPDefCache, ParamCache);
+
   DBuilder.finalize();
 }
 


Index: lib/CodeGen/CGDebugInfo.h
===
--- lib/CodeGen/CGDebugInfo.h
+++ lib/CodeGen/CGDebugInfo.h
@@ -133,6 +133,10 @@
 
   llvm::DenseMap DIFileCache;
   llvm::DenseMap SPCache;
+  /// Cache function definitions relevant to use for parameters mutation
+  /// analysis.
+  llvm::DenseMap SPDefCache;
+  llvm::DenseMap ParamCache;
   /// Cache declarations relevant to DW_TAG_imported_declarations (C++
   /// using declarations) that aren't covered by other more specific caches.
   llvm::DenseMap DeclCache;
Index: lib/CodeGen/CGDebugInfo.cpp
===
--- lib/CodeGen/CGDebugInfo.cpp
+++ lib/CodeGen/CGDebugInfo.cpp
@@ -18,6 +18,7 @@
 #include "CodeGenFunction.h"
 #include "CodeGenModule.h"
 #include "ConstantEmitter.h"
+#include "clang/Analysis/Analyses/ExprMutationAnalyzer.h"
 #include "clang/AST/ASTContext.h"
 #include "clang/AST/DeclFriend.h"
 #include "clang/AST/DeclObjC.h"
@@ -3515,6 +3516,12 @@
   if (HasDecl && isa(D))
 DeclCache[D->getCanonicalDecl()].reset(SP);
 
+  // We use the SPDefCache only in the case when the debug entry values option
+  // is set, in order to speed up parameters modification analysis.
+  if (CGM.getCodeGenOpts().EnableDebugEntryValues && HasDecl &&
+  isa(D))
+SPDefCache[cast(D)].reset(SP);
+
   if (CGM.getCodeGenOpts().DwarfVersion >= 5) {
 // Starting with DWARF V5 method declarations are emitted as children of
 // the 

[PATCH] D58035: [clang/DIVar] Emit flag for params that have unchanged values

[Adrian Prantl via Phabricator via cfe-commits]

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

Couple more nitpicks, but otherwise good.


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

https://reviews.llvm.org/D58035



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

[PATCH] D58035: [clang/DIVar] Emit flag for params that have unchanged values

[Adrian Prantl via Phabricator via cfe-commits]

aprantl added inline comments.



Comment at: lib/CodeGen/CGDebugInfo.cpp:4468
 }
 
+static void analyzeParametersModification(

/// Analyzes each function parameter to determine whether it is constant 
throughout the  function body.



Comment at: lib/CodeGen/CGDebugInfo.cpp:4478
+  ExprMutationAnalyzer FuncAnalyzer(*FuncBody, Ctx);
+  if (!FuncAnalyzer.isMutated(Parm)) {
+auto I = ParamCache.find(Parm);

```
if (FuncAnalyzer.isMutated(Parm))
  continue;
```



Comment at: lib/CodeGen/CGDebugInfo.cpp:4483
+  DIParm->setIsNotModified();
+}
+  }

should the else be an assertion or are there legitimate failure cases?


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

https://reviews.llvm.org/D58035



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

[PATCH] D58035: [clang/DIVar] Emit flag for params that have unchanged values

[Djordje Todorovic via Phabricator via cfe-commits]

djtodoro updated this revision to Diff 200486.
djtodoro added a comment.

-Use `SPCache` instead of `DeclCache`
-Refactor the code by addressing suggestions


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

https://reviews.llvm.org/D58035

Files:
  lib/CodeGen/CGDebugInfo.cpp
  lib/CodeGen/CGDebugInfo.h


Index: lib/CodeGen/CGDebugInfo.h
===
--- lib/CodeGen/CGDebugInfo.h
+++ lib/CodeGen/CGDebugInfo.h
@@ -133,6 +133,7 @@
 
   llvm::DenseMap DIFileCache;
   llvm::DenseMap SPCache;
+  llvm::DenseMap ParamCache;
   /// Cache declarations relevant to DW_TAG_imported_declarations (C++
   /// using declarations) that aren't covered by other more specific caches.
   llvm::DenseMap DeclCache;
Index: lib/CodeGen/CGDebugInfo.cpp
===
--- lib/CodeGen/CGDebugInfo.cpp
+++ lib/CodeGen/CGDebugInfo.cpp
@@ -18,6 +18,7 @@
 #include "CodeGenFunction.h"
 #include "CodeGenModule.h"
 #include "ConstantEmitter.h"
+#include "clang/Analysis/Analyses/ExprMutationAnalyzer.h"
 #include "clang/AST/ASTContext.h"
 #include "clang/AST/DeclFriend.h"
 #include "clang/AST/DeclObjC.h"
@@ -3515,6 +3516,12 @@
   if (HasDecl && isa(D))
 DeclCache[D->getCanonicalDecl()].reset(SP);
 
+  // We use the SPCache only in the case when the debug entry values option is
+  // set, in order to speed up parameters modification analysis.
+  if (CGM.getCodeGenOpts().EnableDebugEntryValues && HasDecl &&
+  isa(D))
+SPCache[cast(D)->getCanonicalDecl()].reset(SP);
+
   if (CGM.getCodeGenOpts().DwarfVersion >= 5) {
 // Starting with DWARF V5 method declarations are emitted as children of
 // the interface type.
@@ -3880,6 +3887,11 @@
  llvm::DebugLoc::get(Line, Column, Scope, 
CurInlinedAt),
  Builder.GetInsertBlock());
 
+  if (CGM.getCodeGenOpts().EnableDebugEntryValues && ArgNo) {
+if (auto *PD = dyn_cast(VD))
+  ParamCache[PD].reset(D);
+  }
+
   return D;
 }
 
@@ -4454,6 +4466,26 @@
   TheCU->setDWOId(Signature);
 }
 
+static void analyzeParametersModification(
+ASTContext ,
+llvm::DenseMap ,
+llvm::DenseMap ) {
+  for (auto  : SPCache) {
+auto *FD = SP.first;
+const Stmt *FuncBody = (*FD).getBody();
+for (auto Parm : FD->parameters()) {
+  ExprMutationAnalyzer FuncAnalyzer(*FuncBody, Ctx);
+  if (!FuncAnalyzer.isMutated(Parm)) {
+auto I = ParamCache.find(Parm);
+if (I != ParamCache.end()) {
+  auto *DIParm = cast(I->second);
+  DIParm->setIsNotModified();
+}
+  }
+}
+  }
+}
+
 void CGDebugInfo::finalize() {
   // Creating types might create further types - invalidating the current
   // element and the size(), so don't cache/reference them.
@@ -4526,6 +4558,10 @@
 if (auto MD = TypeCache[RT])
   DBuilder.retainType(cast(MD));
 
+  if (CGM.getCodeGenOpts().EnableDebugEntryValues)
+// This will be used to emit debug entry values.
+analyzeParametersModification(CGM.getContext(), SPCache, ParamCache);
+
   DBuilder.finalize();
 }
 


Index: lib/CodeGen/CGDebugInfo.h
===
--- lib/CodeGen/CGDebugInfo.h
+++ lib/CodeGen/CGDebugInfo.h
@@ -133,6 +133,7 @@
 
   llvm::DenseMap DIFileCache;
   llvm::DenseMap SPCache;
+  llvm::DenseMap ParamCache;
   /// Cache declarations relevant to DW_TAG_imported_declarations (C++
   /// using declarations) that aren't covered by other more specific caches.
   llvm::DenseMap DeclCache;
Index: lib/CodeGen/CGDebugInfo.cpp
===
--- lib/CodeGen/CGDebugInfo.cpp
+++ lib/CodeGen/CGDebugInfo.cpp
@@ -18,6 +18,7 @@
 #include "CodeGenFunction.h"
 #include "CodeGenModule.h"
 #include "ConstantEmitter.h"
+#include "clang/Analysis/Analyses/ExprMutationAnalyzer.h"
 #include "clang/AST/ASTContext.h"
 #include "clang/AST/DeclFriend.h"
 #include "clang/AST/DeclObjC.h"
@@ -3515,6 +3516,12 @@
   if (HasDecl && isa(D))
 DeclCache[D->getCanonicalDecl()].reset(SP);
 
+  // We use the SPCache only in the case when the debug entry values option is
+  // set, in order to speed up parameters modification analysis.
+  if (CGM.getCodeGenOpts().EnableDebugEntryValues && HasDecl &&
+  isa(D))
+SPCache[cast(D)->getCanonicalDecl()].reset(SP);
+
   if (CGM.getCodeGenOpts().DwarfVersion >= 5) {
 // Starting with DWARF V5 method declarations are emitted as children of
 // the interface type.
@@ -3880,6 +3887,11 @@
  llvm::DebugLoc::get(Line, Column, Scope, CurInlinedAt),
  Builder.GetInsertBlock());
 
+  if (CGM.getCodeGenOpts().EnableDebugEntryValues && ArgNo) {
+if (auto *PD = dyn_cast(VD))
+  ParamCache[PD].reset(D);
+  }
+
   return D;
 }
 
@@ -4454,6 +4466,26 @@
   TheCU->setDWOId(Signature);
 }
 
+static void analyzeParametersModification(
+

[PATCH] D58035: [clang/DIVar] Emit flag for params that have unchanged values

[Adrian Prantl via Phabricator via cfe-commits]

aprantl added inline comments.



Comment at: lib/CodeGen/CGDebugInfo.cpp:4537
+  CGM.getLangOpts().Optimize) {
+for (auto  : DeclCache) {
+  auto *D = SP.first;

djtodoro wrote:
> aprantl wrote:
> > djtodoro wrote:
> > > aprantl wrote:
> > > > Just looking at the type declarations in CGDebugInfo.h: Why not iterate 
> > > > over the `SPCache`  directly? Shouldn't that contain all Function 
> > > > declarations only?
> > > I tried it, but `SPCache` is empty at this point.
> > Where is it emptied? Just grepping through CGDebugInfo did not make this 
> > obvious to me.
> The `SPCache` actually gets filled only in the case of `CXXMemberFunction`.
> In the other cases of `SP` production there is only filling of `DeclCache`.
> Should we use it like this or ?
If the number of entries in the DeclCache is much larger than the size of 
SPCache, we should keep them separate to speed up this loop. Otherwise we 
should join them to conserve memory.


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

https://reviews.llvm.org/D58035



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

[PATCH] D58035: [clang/DIVar] Emit flag for params that have unchanged values

[Djordje Todorovic via Phabricator via cfe-commits]

djtodoro marked 3 inline comments as done.
djtodoro added inline comments.



Comment at: lib/CodeGen/CGDebugInfo.cpp:4537
+  CGM.getLangOpts().Optimize) {
+for (auto  : DeclCache) {
+  auto *D = SP.first;

aprantl wrote:
> djtodoro wrote:
> > aprantl wrote:
> > > Just looking at the type declarations in CGDebugInfo.h: Why not iterate 
> > > over the `SPCache`  directly? Shouldn't that contain all Function 
> > > declarations only?
> > I tried it, but `SPCache` is empty at this point.
> Where is it emptied? Just grepping through CGDebugInfo did not make this 
> obvious to me.
The `SPCache` actually gets filled only in the case of `CXXMemberFunction`.
In the other cases of `SP` production there is only filling of `DeclCache`.
Should we use it like this or ?



Comment at: lib/CodeGen/CGDebugInfo.cpp:3885
+  if (CGM.getCodeGenOpts().EnableDebugEntryValues &&
+  CGM.getLangOpts().Optimize && ArgNo) {
+if (auto *PD = dyn_cast(VD))

aprantl wrote:
> We shouldn't query CGM.getLangOpts().Optimize. If we don't want this to 
> happen at -O0, we shouldn't set EnableDebugEntryValues at a higher level 
> (Driver, CompilerInvocation, ...) ..Otherwise inevitably someone will query 
> one but not the other and things will go out of sync.
I agree. Thanks!



Comment at: lib/CodeGen/CGDebugInfo.cpp:4535
   DBuilder.retainType(cast(MD));
 
+  if (CGM.getCodeGenOpts().EnableDebugEntryValues &&

aprantl wrote:
> Please add either a top-level comment about what this block is doing or 
> perhaps factor this out into a descriptively named static function.
Sure.


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

https://reviews.llvm.org/D58035



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

[PATCH] D58035: [clang/DIVar] Emit flag for params that have unchanged values

[Adrian Prantl via Phabricator via cfe-commits]

aprantl added inline comments.



Comment at: lib/CodeGen/CGDebugInfo.cpp:4537
+  CGM.getLangOpts().Optimize) {
+for (auto  : DeclCache) {
+  auto *D = SP.first;

djtodoro wrote:
> aprantl wrote:
> > Just looking at the type declarations in CGDebugInfo.h: Why not iterate 
> > over the `SPCache`  directly? Shouldn't that contain all Function 
> > declarations only?
> I tried it, but `SPCache` is empty at this point.
Where is it emptied? Just grepping through CGDebugInfo did not make this 
obvious to me.



Comment at: lib/CodeGen/CGDebugInfo.cpp:3885
+  if (CGM.getCodeGenOpts().EnableDebugEntryValues &&
+  CGM.getLangOpts().Optimize && ArgNo) {
+if (auto *PD = dyn_cast(VD))

We shouldn't query CGM.getLangOpts().Optimize. If we don't want this to happen 
at -O0, we shouldn't set EnableDebugEntryValues at a higher level (Driver, 
CompilerInvocation, ...) ..Otherwise inevitably someone will query one but not 
the other and things will go out of sync.



Comment at: lib/CodeGen/CGDebugInfo.cpp:4535
   DBuilder.retainType(cast(MD));
 
+  if (CGM.getCodeGenOpts().EnableDebugEntryValues &&

Please add either a top-level comment about what this block is doing or perhaps 
factor this out into a descriptively named static function.


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

https://reviews.llvm.org/D58035



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

[PATCH] D58035: [clang/DIVar] Emit flag for params that have unchanged values

[Djordje Todorovic via Phabricator via cfe-commits]

djtodoro updated this revision to Diff 199788.
djtodoro added a comment.

-Careful use of `dyn_cast`
-Fill the `ParamCache` only in the case of `EnableDebugEntryValues` option


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

https://reviews.llvm.org/D58035

Files:
  lib/CodeGen/CGDebugInfo.cpp
  lib/CodeGen/CGDebugInfo.h


Index: lib/CodeGen/CGDebugInfo.h
===
--- lib/CodeGen/CGDebugInfo.h
+++ lib/CodeGen/CGDebugInfo.h
@@ -133,6 +133,7 @@
 
   llvm::DenseMap DIFileCache;
   llvm::DenseMap SPCache;
+  llvm::DenseMap ParmCache;
   /// Cache declarations relevant to DW_TAG_imported_declarations (C++
   /// using declarations) that aren't covered by other more specific caches.
   llvm::DenseMap DeclCache;
Index: lib/CodeGen/CGDebugInfo.cpp
===
--- lib/CodeGen/CGDebugInfo.cpp
+++ lib/CodeGen/CGDebugInfo.cpp
@@ -18,6 +18,7 @@
 #include "CodeGenFunction.h"
 #include "CodeGenModule.h"
 #include "ConstantEmitter.h"
+#include "clang/Analysis/Analyses/ExprMutationAnalyzer.h"
 #include "clang/AST/ASTContext.h"
 #include "clang/AST/DeclFriend.h"
 #include "clang/AST/DeclObjC.h"
@@ -3880,6 +3881,12 @@
  llvm::DebugLoc::get(Line, Column, Scope, 
CurInlinedAt),
  Builder.GetInsertBlock());
 
+  if (CGM.getCodeGenOpts().EnableDebugEntryValues &&
+  CGM.getLangOpts().Optimize && ArgNo) {
+if (auto *PD = dyn_cast(VD))
+  ParmCache[PD].reset(D);
+  }
+
   return D;
 }
 
@@ -4526,6 +4533,25 @@
 if (auto MD = TypeCache[RT])
   DBuilder.retainType(cast(MD));
 
+  if (CGM.getCodeGenOpts().EnableDebugEntryValues &&
+  CGM.getLangOpts().Optimize) {
+for (auto  : DeclCache) {
+  auto *D = SP.first;
+  if (const auto *FD = dyn_cast_or_null(D)) {
+const Stmt *FuncBody = (*FD).getBody();
+for (auto Parm : FD->parameters()) {
+  ExprMutationAnalyzer FuncAnalyzer(*FuncBody, CGM.getContext());
+  if (!FuncAnalyzer.isMutated(Parm)) {
+auto I = ParmCache.find(Parm);
+if (I != ParmCache.end()) {
+  auto *DIParm = cast(I->second);
+  DIParm->setIsNotModified();
+}
+  }
+}
+  }
+}
+  }
   DBuilder.finalize();
 }
 


Index: lib/CodeGen/CGDebugInfo.h
===
--- lib/CodeGen/CGDebugInfo.h
+++ lib/CodeGen/CGDebugInfo.h
@@ -133,6 +133,7 @@
 
   llvm::DenseMap DIFileCache;
   llvm::DenseMap SPCache;
+  llvm::DenseMap ParmCache;
   /// Cache declarations relevant to DW_TAG_imported_declarations (C++
   /// using declarations) that aren't covered by other more specific caches.
   llvm::DenseMap DeclCache;
Index: lib/CodeGen/CGDebugInfo.cpp
===
--- lib/CodeGen/CGDebugInfo.cpp
+++ lib/CodeGen/CGDebugInfo.cpp
@@ -18,6 +18,7 @@
 #include "CodeGenFunction.h"
 #include "CodeGenModule.h"
 #include "ConstantEmitter.h"
+#include "clang/Analysis/Analyses/ExprMutationAnalyzer.h"
 #include "clang/AST/ASTContext.h"
 #include "clang/AST/DeclFriend.h"
 #include "clang/AST/DeclObjC.h"
@@ -3880,6 +3881,12 @@
  llvm::DebugLoc::get(Line, Column, Scope, CurInlinedAt),
  Builder.GetInsertBlock());
 
+  if (CGM.getCodeGenOpts().EnableDebugEntryValues &&
+  CGM.getLangOpts().Optimize && ArgNo) {
+if (auto *PD = dyn_cast(VD))
+  ParmCache[PD].reset(D);
+  }
+
   return D;
 }
 
@@ -4526,6 +4533,25 @@
 if (auto MD = TypeCache[RT])
   DBuilder.retainType(cast(MD));
 
+  if (CGM.getCodeGenOpts().EnableDebugEntryValues &&
+  CGM.getLangOpts().Optimize) {
+for (auto  : DeclCache) {
+  auto *D = SP.first;
+  if (const auto *FD = dyn_cast_or_null(D)) {
+const Stmt *FuncBody = (*FD).getBody();
+for (auto Parm : FD->parameters()) {
+  ExprMutationAnalyzer FuncAnalyzer(*FuncBody, CGM.getContext());
+  if (!FuncAnalyzer.isMutated(Parm)) {
+auto I = ParmCache.find(Parm);
+if (I != ParmCache.end()) {
+  auto *DIParm = cast(I->second);
+  DIParm->setIsNotModified();
+}
+  }
+}
+  }
+}
+  }
   DBuilder.finalize();
 }
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D58035: [clang/DIVar] Emit flag for params that have unchanged values

[Djordje Todorovic via Phabricator via cfe-commits]

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



Comment at: lib/CodeGen/CGDebugInfo.cpp:3885
+  if (ArgNo) {
+auto *PD = dyn_cast(VD);
+ParmCache[PD].reset(D);

aprantl wrote:
> A `dyn_cast` followed by an unconditional use seems strange. I would expect 
> either a `cast` or an `if (PD)` check.
Yes, thanks for this!


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

https://reviews.llvm.org/D58035



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

[PATCH] D58035: [clang/DIVar] Emit flag for params that have unchanged values

[Adrian Prantl via Phabricator via cfe-commits]

aprantl added inline comments.



Comment at: lib/CodeGen/CGDebugInfo.cpp:3885
+  if (ArgNo) {
+auto *PD = dyn_cast(VD);
+ParmCache[PD].reset(D);

A `dyn_cast` followed by an unconditional use seems strange. I would expect 
either a `cast` or an `if (PD)` check.


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

https://reviews.llvm.org/D58035



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

[PATCH] D58035: [clang/DIVar] Emit flag for params that have unchanged values

[Djordje Todorovic via Phabricator via cfe-commits]

djtodoro updated this revision to Diff 199214.
djtodoro added a comment.

-Rebase


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

https://reviews.llvm.org/D58035

Files:
  lib/CodeGen/CGDebugInfo.cpp
  lib/CodeGen/CGDebugInfo.h


Index: lib/CodeGen/CGDebugInfo.h
===
--- lib/CodeGen/CGDebugInfo.h
+++ lib/CodeGen/CGDebugInfo.h
@@ -133,6 +133,7 @@
 
   llvm::DenseMap DIFileCache;
   llvm::DenseMap SPCache;
+  llvm::DenseMap ParmCache;
   /// Cache declarations relevant to DW_TAG_imported_declarations (C++
   /// using declarations) that aren't covered by other more specific caches.
   llvm::DenseMap DeclCache;
Index: lib/CodeGen/CGDebugInfo.cpp
===
--- lib/CodeGen/CGDebugInfo.cpp
+++ lib/CodeGen/CGDebugInfo.cpp
@@ -18,6 +18,7 @@
 #include "CodeGenFunction.h"
 #include "CodeGenModule.h"
 #include "ConstantEmitter.h"
+#include "clang/Analysis/Analyses/ExprMutationAnalyzer.h"
 #include "clang/AST/ASTContext.h"
 #include "clang/AST/DeclFriend.h"
 #include "clang/AST/DeclObjC.h"
@@ -3880,6 +3881,11 @@
  llvm::DebugLoc::get(Line, Column, Scope, 
CurInlinedAt),
  Builder.GetInsertBlock());
 
+  if (ArgNo) {
+auto *PD = dyn_cast(VD);
+ParmCache[PD].reset(D);
+  }
+
   return D;
 }
 
@@ -4526,6 +4532,25 @@
 if (auto MD = TypeCache[RT])
   DBuilder.retainType(cast(MD));
 
+  if (CGM.getCodeGenOpts().EnableDebugEntryValues &&
+  CGM.getLangOpts().Optimize) {
+for (auto  : DeclCache) {
+  auto *D = SP.first;
+  if (const auto *FD = dyn_cast_or_null(D)) {
+const Stmt *FuncBody = (*FD).getBody();
+for (auto Parm : FD->parameters()) {
+  ExprMutationAnalyzer FuncAnalyzer(*FuncBody, CGM.getContext());
+  if (!FuncAnalyzer.isMutated(Parm)) {
+auto I = ParmCache.find(Parm);
+if (I != ParmCache.end()) {
+  auto *DIParm = cast(I->second);
+  DIParm->setIsNotModified();
+}
+  }
+}
+  }
+}
+  }
   DBuilder.finalize();
 }
 


Index: lib/CodeGen/CGDebugInfo.h
===
--- lib/CodeGen/CGDebugInfo.h
+++ lib/CodeGen/CGDebugInfo.h
@@ -133,6 +133,7 @@
 
   llvm::DenseMap DIFileCache;
   llvm::DenseMap SPCache;
+  llvm::DenseMap ParmCache;
   /// Cache declarations relevant to DW_TAG_imported_declarations (C++
   /// using declarations) that aren't covered by other more specific caches.
   llvm::DenseMap DeclCache;
Index: lib/CodeGen/CGDebugInfo.cpp
===
--- lib/CodeGen/CGDebugInfo.cpp
+++ lib/CodeGen/CGDebugInfo.cpp
@@ -18,6 +18,7 @@
 #include "CodeGenFunction.h"
 #include "CodeGenModule.h"
 #include "ConstantEmitter.h"
+#include "clang/Analysis/Analyses/ExprMutationAnalyzer.h"
 #include "clang/AST/ASTContext.h"
 #include "clang/AST/DeclFriend.h"
 #include "clang/AST/DeclObjC.h"
@@ -3880,6 +3881,11 @@
  llvm::DebugLoc::get(Line, Column, Scope, CurInlinedAt),
  Builder.GetInsertBlock());
 
+  if (ArgNo) {
+auto *PD = dyn_cast(VD);
+ParmCache[PD].reset(D);
+  }
+
   return D;
 }
 
@@ -4526,6 +4532,25 @@
 if (auto MD = TypeCache[RT])
   DBuilder.retainType(cast(MD));
 
+  if (CGM.getCodeGenOpts().EnableDebugEntryValues &&
+  CGM.getLangOpts().Optimize) {
+for (auto  : DeclCache) {
+  auto *D = SP.first;
+  if (const auto *FD = dyn_cast_or_null(D)) {
+const Stmt *FuncBody = (*FD).getBody();
+for (auto Parm : FD->parameters()) {
+  ExprMutationAnalyzer FuncAnalyzer(*FuncBody, CGM.getContext());
+  if (!FuncAnalyzer.isMutated(Parm)) {
+auto I = ParmCache.find(Parm);
+if (I != ParmCache.end()) {
+  auto *DIParm = cast(I->second);
+  DIParm->setIsNotModified();
+}
+  }
+}
+  }
+}
+  }
   DBuilder.finalize();
 }
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D58035: [clang/DIVar] Emit flag for params that have unchanged values

[Djordje Todorovic via Phabricator via cfe-commits]

djtodoro updated this revision to Diff 195359.
djtodoro added a comment.

-Run clang-format
-Use `cast` instead of '`dyn_cast`'


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

https://reviews.llvm.org/D58035

Files:
  lib/CodeGen/CGDebugInfo.cpp
  lib/CodeGen/CGDebugInfo.h


Index: lib/CodeGen/CGDebugInfo.h
===
--- lib/CodeGen/CGDebugInfo.h
+++ lib/CodeGen/CGDebugInfo.h
@@ -133,6 +133,7 @@
 
   llvm::DenseMap DIFileCache;
   llvm::DenseMap SPCache;
+  llvm::DenseMap ParmCache;
   /// Cache declarations relevant to DW_TAG_imported_declarations (C++
   /// using declarations) that aren't covered by other more specific caches.
   llvm::DenseMap DeclCache;
Index: lib/CodeGen/CGDebugInfo.cpp
===
--- lib/CodeGen/CGDebugInfo.cpp
+++ lib/CodeGen/CGDebugInfo.cpp
@@ -18,6 +18,7 @@
 #include "CodeGenFunction.h"
 #include "CodeGenModule.h"
 #include "ConstantEmitter.h"
+#include "clang/Analysis/Analyses/ExprMutationAnalyzer.h"
 #include "clang/AST/ASTContext.h"
 #include "clang/AST/DeclFriend.h"
 #include "clang/AST/DeclObjC.h"
@@ -3880,6 +3881,11 @@
  llvm::DebugLoc::get(Line, Column, Scope, 
CurInlinedAt),
  Builder.GetInsertBlock());
 
+  if (ArgNo) {
+auto *PD = dyn_cast(VD);
+ParmCache[PD].reset(D);
+  }
+
   return D;
 }
 
@@ -4526,6 +4532,25 @@
 if (auto MD = TypeCache[RT])
   DBuilder.retainType(cast(MD));
 
+  if (CGM.getCodeGenOpts().EnableParamEntryValues &&
+  CGM.getLangOpts().Optimize) {
+for (auto  : DeclCache) {
+  auto *D = SP.first;
+  if (const auto *FD = dyn_cast_or_null(D)) {
+const Stmt *FuncBody = (*FD).getBody();
+for (auto Parm : FD->parameters()) {
+  ExprMutationAnalyzer FuncAnalyzer(*FuncBody, CGM.getContext());
+  if (!FuncAnalyzer.isMutated(Parm)) {
+auto I = ParmCache.find(Parm);
+if (I != ParmCache.end()) {
+  auto *DIParm = cast(I->second);
+  DIParm->setIsNotModified();
+}
+  }
+}
+  }
+}
+  }
   DBuilder.finalize();
 }
 


Index: lib/CodeGen/CGDebugInfo.h
===
--- lib/CodeGen/CGDebugInfo.h
+++ lib/CodeGen/CGDebugInfo.h
@@ -133,6 +133,7 @@
 
   llvm::DenseMap DIFileCache;
   llvm::DenseMap SPCache;
+  llvm::DenseMap ParmCache;
   /// Cache declarations relevant to DW_TAG_imported_declarations (C++
   /// using declarations) that aren't covered by other more specific caches.
   llvm::DenseMap DeclCache;
Index: lib/CodeGen/CGDebugInfo.cpp
===
--- lib/CodeGen/CGDebugInfo.cpp
+++ lib/CodeGen/CGDebugInfo.cpp
@@ -18,6 +18,7 @@
 #include "CodeGenFunction.h"
 #include "CodeGenModule.h"
 #include "ConstantEmitter.h"
+#include "clang/Analysis/Analyses/ExprMutationAnalyzer.h"
 #include "clang/AST/ASTContext.h"
 #include "clang/AST/DeclFriend.h"
 #include "clang/AST/DeclObjC.h"
@@ -3880,6 +3881,11 @@
  llvm::DebugLoc::get(Line, Column, Scope, CurInlinedAt),
  Builder.GetInsertBlock());
 
+  if (ArgNo) {
+auto *PD = dyn_cast(VD);
+ParmCache[PD].reset(D);
+  }
+
   return D;
 }
 
@@ -4526,6 +4532,25 @@
 if (auto MD = TypeCache[RT])
   DBuilder.retainType(cast(MD));
 
+  if (CGM.getCodeGenOpts().EnableParamEntryValues &&
+  CGM.getLangOpts().Optimize) {
+for (auto  : DeclCache) {
+  auto *D = SP.first;
+  if (const auto *FD = dyn_cast_or_null(D)) {
+const Stmt *FuncBody = (*FD).getBody();
+for (auto Parm : FD->parameters()) {
+  ExprMutationAnalyzer FuncAnalyzer(*FuncBody, CGM.getContext());
+  if (!FuncAnalyzer.isMutated(Parm)) {
+auto I = ParmCache.find(Parm);
+if (I != ParmCache.end()) {
+  auto *DIParm = cast(I->second);
+  DIParm->setIsNotModified();
+}
+  }
+}
+  }
+}
+  }
   DBuilder.finalize();
 }
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D58035: [clang/DIVar] Emit flag for params that have unchanged values

[Djordje Todorovic via Phabricator via cfe-commits]

djtodoro marked 3 inline comments as done.
djtodoro added inline comments.



Comment at: lib/CodeGen/CGDebugInfo.cpp:4537
+  CGM.getLangOpts().Optimize) {
+for (auto  : DeclCache) {
+  auto *D = SP.first;

aprantl wrote:
> Just looking at the type declarations in CGDebugInfo.h: Why not iterate over 
> the `SPCache`  directly? Shouldn't that contain all Function declarations 
> only?
I tried it, but `SPCache` is empty at this point.


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

https://reviews.llvm.org/D58035



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

[PATCH] D58035: [clang/DIVar] Emit flag for params that have unchanged values

[Adrian Prantl via Phabricator via cfe-commits]

aprantl added inline comments.



Comment at: lib/CodeGen/CGDebugInfo.cpp:4537
+  CGM.getLangOpts().Optimize) {
+for (auto  : DeclCache) {
+  auto *D = SP.first;

Just looking at the type declarations in CGDebugInfo.h: Why not iterate over 
the `SPCache`  directly? Shouldn't that contain all Function declarations only?



Comment at: lib/CodeGen/CGDebugInfo.cpp:4541
+const Stmt *FuncBody = (*FD).getBody();
+for(auto Parm : FD->parameters()) {
+  ExprMutationAnalyzer FuncAnalyzer(*FuncBody, CGM.getContext());

clang-format please



Comment at: lib/CodeGen/CGDebugInfo.cpp:4546
+if (I != ParmCache.end()) {
+  auto *DIParm = dyn_cast(I->second);
+  DIParm->setIsNotModified();

Could this be a `cast<>`?


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

https://reviews.llvm.org/D58035



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

[PATCH] D58035: [clang/DIVar] Emit flag for params that have unchanged values

[Djordje Todorovic via Phabricator via cfe-commits]

djtodoro updated this revision to Diff 195192.
djtodoro added a comment.

-Rebase
-Use `ExprMutationAnalyzer` for parameter's modification check


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

https://reviews.llvm.org/D58035

Files:
  lib/CodeGen/CGDebugInfo.cpp
  lib/CodeGen/CGDebugInfo.h


Index: lib/CodeGen/CGDebugInfo.h
===
--- lib/CodeGen/CGDebugInfo.h
+++ lib/CodeGen/CGDebugInfo.h
@@ -133,6 +133,7 @@
 
   llvm::DenseMap DIFileCache;
   llvm::DenseMap SPCache;
+  llvm::DenseMap ParmCache;
   /// Cache declarations relevant to DW_TAG_imported_declarations (C++
   /// using declarations) that aren't covered by other more specific caches.
   llvm::DenseMap DeclCache;
Index: lib/CodeGen/CGDebugInfo.cpp
===
--- lib/CodeGen/CGDebugInfo.cpp
+++ lib/CodeGen/CGDebugInfo.cpp
@@ -18,6 +18,7 @@
 #include "CodeGenFunction.h"
 #include "CodeGenModule.h"
 #include "ConstantEmitter.h"
+#include "clang/Analysis/Analyses/ExprMutationAnalyzer.h"
 #include "clang/AST/ASTContext.h"
 #include "clang/AST/DeclFriend.h"
 #include "clang/AST/DeclObjC.h"
@@ -3880,6 +3881,11 @@
  llvm::DebugLoc::get(Line, Column, Scope, 
CurInlinedAt),
  Builder.GetInsertBlock());
 
+  if (ArgNo) {
+auto *PD = dyn_cast(VD);
+ParmCache[PD].reset(D);
+  }
+
   return D;
 }
 
@@ -4526,6 +4532,25 @@
 if (auto MD = TypeCache[RT])
   DBuilder.retainType(cast(MD));
 
+  if (CGM.getCodeGenOpts().EnableParamEntryValues &&
+  CGM.getLangOpts().Optimize) {
+for (auto  : DeclCache) {
+  auto *D = SP.first;
+  if (const auto *FD = dyn_cast_or_null(D)) {
+const Stmt *FuncBody = (*FD).getBody();
+for(auto Parm : FD->parameters()) {
+  ExprMutationAnalyzer FuncAnalyzer(*FuncBody, CGM.getContext());
+  if (!FuncAnalyzer.isMutated(Parm)) {
+auto I = ParmCache.find(Parm);
+if (I != ParmCache.end()) {
+  auto *DIParm = dyn_cast(I->second);
+  DIParm->setIsNotModified();
+}
+  }
+}
+  }
+}
+  }
   DBuilder.finalize();
 }
 


Index: lib/CodeGen/CGDebugInfo.h
===
--- lib/CodeGen/CGDebugInfo.h
+++ lib/CodeGen/CGDebugInfo.h
@@ -133,6 +133,7 @@
 
   llvm::DenseMap DIFileCache;
   llvm::DenseMap SPCache;
+  llvm::DenseMap ParmCache;
   /// Cache declarations relevant to DW_TAG_imported_declarations (C++
   /// using declarations) that aren't covered by other more specific caches.
   llvm::DenseMap DeclCache;
Index: lib/CodeGen/CGDebugInfo.cpp
===
--- lib/CodeGen/CGDebugInfo.cpp
+++ lib/CodeGen/CGDebugInfo.cpp
@@ -18,6 +18,7 @@
 #include "CodeGenFunction.h"
 #include "CodeGenModule.h"
 #include "ConstantEmitter.h"
+#include "clang/Analysis/Analyses/ExprMutationAnalyzer.h"
 #include "clang/AST/ASTContext.h"
 #include "clang/AST/DeclFriend.h"
 #include "clang/AST/DeclObjC.h"
@@ -3880,6 +3881,11 @@
  llvm::DebugLoc::get(Line, Column, Scope, CurInlinedAt),
  Builder.GetInsertBlock());
 
+  if (ArgNo) {
+auto *PD = dyn_cast(VD);
+ParmCache[PD].reset(D);
+  }
+
   return D;
 }
 
@@ -4526,6 +4532,25 @@
 if (auto MD = TypeCache[RT])
   DBuilder.retainType(cast(MD));
 
+  if (CGM.getCodeGenOpts().EnableParamEntryValues &&
+  CGM.getLangOpts().Optimize) {
+for (auto  : DeclCache) {
+  auto *D = SP.first;
+  if (const auto *FD = dyn_cast_or_null(D)) {
+const Stmt *FuncBody = (*FD).getBody();
+for(auto Parm : FD->parameters()) {
+  ExprMutationAnalyzer FuncAnalyzer(*FuncBody, CGM.getContext());
+  if (!FuncAnalyzer.isMutated(Parm)) {
+auto I = ParmCache.find(Parm);
+if (I != ParmCache.end()) {
+  auto *DIParm = dyn_cast(I->second);
+  DIParm->setIsNotModified();
+}
+  }
+}
+  }
+}
+  }
   DBuilder.finalize();
 }
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D58035: [clang/DIVar] Emit flag for params that have unchanged values

[Djordje Todorovic via Phabricator via cfe-commits]

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



Comment at: lib/Sema/SemaExpr.cpp:11292
+}
+
+/// Argument's value might be modified, so update the info.

riccibruno wrote:
> Hmm, I don't think that this will work. Suppose that you have an expression 
> like `(a, b) += c`  You want to mark `b` as modified, but not `a`. What is 
> needed I believe is:
> 
> Given an expression `E` find the variable that this expression designate if 
> any, and if it is a function parameter mark it "known to be modified".
> 
> Visiting all of the children just looking for `DeclRefExpr`s is a bit too 
> blunt I believe.
You are right. We thought it is possible implementing this with some basic 
analysis (with low overhead), but in order to support all cases it would take 
much time and it is not good idea.

I tried to use `ExprMutationAnalyzer ` (that @lebedev.ri mentioned) and it 
seems to be useful for this. It seems that it can be used a little bit later 
than in this phase, because it takes ASTContext, but in this stage we have 
ASTContext finished up to node we are currently observing (an expression from 
which we are extracting DeclRef). But, even using it in the right time, it 
seems that `ExprMutationAnalyzer ` does not have support for comma operator. 
I'm trying to extend this analyzer in order to add support for that. ASAP I 
will post new changes.
For the other cases I tried, it works.


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

https://reviews.llvm.org/D58035



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

[PATCH] D58035: [clang/DIVar] Emit flag for params that have unchanged values

[Bruno Ricci via Phabricator via cfe-commits]

riccibruno added inline comments.



Comment at: lib/Sema/SemaExpr.cpp:11292
+}
+
+/// Argument's value might be modified, so update the info.

Hmm, I don't think that this will work. Suppose that you have an expression 
like `(a, b) += c`  You want to mark `b` as modified, but not `a`. What is 
needed I believe is:

Given an expression `E` find the variable that this expression designate if 
any, and if it is a function parameter mark it "known to be modified".

Visiting all of the children just looking for `DeclRefExpr`s is a bit too blunt 
I believe.



Comment at: lib/Sema/SemaExpr.cpp:11316
 IsLV = Expr::MLV_InvalidMessageExpression;
-  if (IsLV == Expr::MLV_Valid)
+  if (IsLV == Expr::MLV_Valid) {
+EmitArgumentsValueModification(E);

> Hmm, we should avoid marking variables modification if this emits an error. 
> But, we should emit it if `CheckForModifiableLvalue`  returns false, since in 
> the case of returning true an error will be emitted.

You are right, I believed for a moment that `CheckForModifiableLvalue` returned 
false on error.


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

https://reviews.llvm.org/D58035



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

[PATCH] D58035: [clang/DIVar] Emit flag for params that have unchanged values

[Djordje Todorovic via Phabricator via cfe-commits]

djtodoro updated this revision to Diff 188110.
djtodoro added a comment.
Herald added a subscriber: jdoerfert.

- Handle all kinds of expressions when mark a param's modification


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

https://reviews.llvm.org/D58035

Files:
  include/clang/AST/Decl.h
  lib/CodeGen/CGDebugInfo.cpp
  lib/Sema/SemaExpr.cpp
  lib/Serialization/ASTReaderDecl.cpp
  lib/Serialization/ASTWriterDecl.cpp
  test/CodeGen/dbginfo-var-change-templates.cpp
  test/CodeGen/debug-info-varchange.c

Index: test/CodeGen/debug-info-varchange.c
===
--- /dev/null
+++ test/CodeGen/debug-info-varchange.c
@@ -0,0 +1,35 @@
+// RUN: %clang -femit-param-entry-values -emit-llvm -S -g %s -o - | FileCheck %s
+
+// CHECK: !DILocalVariable(name: "a", arg: 1, scope: {{.*}}, file: {{.*}}, line: {{.*}}, type: {{.*}}, flags: DIFlagArgumentNotModified)
+// CHECK: !DILocalVariable(name: "b", arg: 2, scope: {{.*}}, file: {{.*}}, line: {{.*}}, type: {{.*}})
+// CHECK: !DILocalVariable(name: "test_s", arg: 3, scope: {{.*}}, file: {{.*}}, line: {{.*}}, type: {{.*}}, flags: DIFlagArgumentNotModified)
+// CHECK: !DILocalVariable(name: "test_s2", arg: 4, scope: {{.*}}, file: {{.*}}, line: {{.*}}, type: {{.*}})
+// CHECK: !DILocalVariable(name: "x", scope: {{.*}}, file: {{.*}}, line: {{.*}}, type: {{.*}})
+// CHECK: !DILocalVariable(name: "y", scope: {{.*}}, file: {{.*}}, line: {{.*}}, type: {{.*}})
+
+typedef struct s {
+  int m;
+  int n;
+}S;
+
+void foo (int a, int b,
+  S test_s, S test_s2)
+{
+  b++;
+  b = a + 1;
+  if (b>4)
+test_s2.m = 434;
+}
+
+int main()
+{
+  S test_s = {4, 5};
+
+  int x = 5;
+  int y = 6;
+
+  foo(x , y, test_s, test_s);
+
+  return 0;
+}
+
Index: test/CodeGen/dbginfo-var-change-templates.cpp
===
--- /dev/null
+++ test/CodeGen/dbginfo-var-change-templates.cpp
@@ -0,0 +1,24 @@
+// RUN: %clang -femit-param-entry-values -emit-llvm -S -g %s -o - | FileCheck %s
+// CHECK: !DILocalVariable(name: "a", arg: 1, scope: {{.*}}, file: {{.*}}, line: {{.*}}, type: {{.*}})
+// CHECK: !DILocalVariable(name: "b", arg: 2, scope: {{.*}}, file: {{.*}}, line: {{.*}}, type: {{.*}}, flags: DIFlagArgumentNotModified)
+
+template 
+__attribute__((noinline))
+T GetMin (T a, T b) {
+  T result;
+  ++a;
+  result = (a < b) ? a : b;
+  return result;
+}
+
+int baa () {
+  int i=5, j=6, k;
+  k=GetMin(i,j);
+
+  if (i == k)
+++k;
+  else
+--k;
+
+  return k;
+}
Index: lib/Serialization/ASTWriterDecl.cpp
===
--- lib/Serialization/ASTWriterDecl.cpp
+++ lib/Serialization/ASTWriterDecl.cpp
@@ -1033,6 +1033,7 @@
   Record.push_back(D->hasUninstantiatedDefaultArg());
   if (D->hasUninstantiatedDefaultArg())
 Record.AddStmt(D->getUninstantiatedDefaultArg());
+  Record.push_back(D->isArgumentKnownToBeModified());
   Code = serialization::DECL_PARM_VAR;
 
   assert(!D->isARCPseudoStrong()); // can be true of ImplicitParamDecl
@@ -1046,6 +1047,7 @@
   !D->isImplicit() &&
   !D->isUsed(false) &&
   !D->isInvalidDecl() &&
+  !D->isArgumentKnownToBeModified() &&
   !D->isReferenced() &&
   D->getAccess() == AS_none &&
   !D->isModulePrivate() &&
@@ -2011,6 +2013,7 @@
   Abv->Add(BitCodeAbbrevOp(0));   // KNRPromoted
   Abv->Add(BitCodeAbbrevOp(0));   // HasInheritedDefaultArg
   Abv->Add(BitCodeAbbrevOp(0));   // HasUninstantiatedDefaultArg
+  Abv->Add(BitCodeAbbrevOp(0));   // IsArgumentKnownToBeModified
   // Type Source Info
   Abv->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::Array));
   Abv->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::VBR, 6)); // TypeLoc
Index: lib/Serialization/ASTReaderDecl.cpp
===
--- lib/Serialization/ASTReaderDecl.cpp
+++ lib/Serialization/ASTReaderDecl.cpp
@@ -1441,7 +1441,7 @@
   PD->ParmVarDeclBits.HasInheritedDefaultArg = Record.readInt();
   if (Record.readInt()) // hasUninstantiatedDefaultArg.
 PD->setUninstantiatedDefaultArg(Record.readExpr());
-
+  PD->ParmVarDeclBits.IsArgumentKnownToBeModified = Record.readInt();
   // FIXME: If this is a redeclaration of a function from another module, handle
   // inheritance of default arguments.
 }
Index: lib/Sema/SemaExpr.cpp
===
--- lib/Sema/SemaExpr.cpp
+++ lib/Sema/SemaExpr.cpp
@@ -16,6 +16,7 @@
 #include "clang/AST/ASTLambda.h"
 #include "clang/AST/ASTMutationListener.h"
 #include "clang/AST/CXXInheritance.h"
+#include "clang/AST/Decl.h"
 #include "clang/AST/DeclObjC.h"
 #include "clang/AST/DeclTemplate.h"
 #include "clang/AST/EvaluatedExprVisitor.h"
@@ -11276,6 +11277,30 @@
 DiagnoseConstAssignment(S, E, Loc);
 }
 
+/// Traverse an expression and find declaration ref, if any.
+static bool HasDeclRef(const 

[PATCH] D58035: [clang/DIVar] Emit flag for params that have unchanged values

[Djordje Todorovic via Phabricator via cfe-commits]

djtodoro marked an inline comment as done.
djtodoro added a comment.

> I'm not quite sure what this differential is about, but i feel like 
> mentioning ExprMutationAnalyzer lib in clang-tidy / clang-tools-extra.



>> Alternatively perhaps you could re-use getMemoryLocation() from D57660 
>> . It would handle for you members, 
>> references, structured bindings +more in a relatively self-contained code.

@lebedev.ri, @riccibruno Thanks for your advices! We'll check this also.




Comment at: lib/Sema/SemaExpr.cpp:11301
+EmitArgumentsValueModification(E);
+
   SourceLocation OrigLoc = Loc;

lebedev.ri wrote:
> riccibruno wrote:
> > Comments:
> > 
> > 1. Shouldn't you mark the variable to be modified only if 
> > `CheckForModifiableLvalue` returns true ?
> > 
> > 2. I think that you need to handle way more than just member expressions. 
> > For example are you handling `(x, y)` (comma operator) ? But hard-coding 
> > every cases here is probably not ideal. It would be nice if there was 
> > already some code somewhere that could help you do this.
> > 
> > 3. I believe that a `MemberExpr` has always a base. Similarly 
> > `DeclRefExpr`s always have a referenced declaration (so you can omit the 
> > `if`).
> I'm not quite sure what this differential is about, but i feel like 
> mentioning ExprMutationAnalyzer lib in clang-tidy / clang-tools-extra.
@riccibruno Please find inlined replies:

>1. Shouldn't you mark the variable to be modified only if 
>CheckForModifiableLvalue returns true ?

Hmm, we should avoid marking variables modification if this emits an error. 
But, we should emit it if `CheckForModifiableLvalue ` returns `false`, since in 
the case of returning `true` an error will be emitted.

>2. I think that you need to handle way more than just member expressions. For 
>example are you handling (x, y) (comma operator) ? But hard-coding every cases 
>here is probably not ideal. It would be nice if there was already some code 
>somewhere that could help you do this.

Hard coding all cases is not good idea. I agree. Since we are only looking for 
declaration references, we could make just a function that traverses any `Expr` 
and find declaration ref.

>3. I believe that a MemberExpr has always a base. Similarly DeclRefExprs 
>always have a referenced declaration (so you can omit the if).

I think you are right. Thanks!




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

https://reviews.llvm.org/D58035



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

[PATCH] D58035: [clang/DIVar] Emit flag for params that have unchanged values

[Bruno Ricci via Phabricator via cfe-commits]

riccibruno added a comment.

> I'm not quite sure what this differential is about, but i feel like 
> mentioning ExprMutationAnalyzer lib in clang-tidy / clang-tools-extra.

Alternatively perhaps you could re-use `getMemoryLocation()` from D57660 
. It would handle for you members, references, 
structured bindings +more in a relatively self-contained code.


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

https://reviews.llvm.org/D58035



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

[PATCH] D58035: [clang/DIVar] Emit flag for params that have unchanged values

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added inline comments.



Comment at: lib/Sema/SemaExpr.cpp:11301
+EmitArgumentsValueModification(E);
+
   SourceLocation OrigLoc = Loc;

riccibruno wrote:
> Comments:
> 
> 1. Shouldn't you mark the variable to be modified only if 
> `CheckForModifiableLvalue` returns true ?
> 
> 2. I think that you need to handle way more than just member expressions. For 
> example are you handling `(x, y)` (comma operator) ? But hard-coding every 
> cases here is probably not ideal. It would be nice if there was already some 
> code somewhere that could help you do this.
> 
> 3. I believe that a `MemberExpr` has always a base. Similarly `DeclRefExpr`s 
> always have a referenced declaration (so you can omit the `if`).
I'm not quite sure what this differential is about, but i feel like mentioning 
ExprMutationAnalyzer lib in clang-tidy / clang-tools-extra.


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

https://reviews.llvm.org/D58035



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

[PATCH] D58035: [clang/DIVar] Emit flag for params that have unchanged values

[Bruno Ricci via Phabricator via cfe-commits]

riccibruno added inline comments.



Comment at: lib/Sema/SemaExpr.cpp:11301
+EmitArgumentsValueModification(E);
+
   SourceLocation OrigLoc = Loc;

Comments:

1. Shouldn't you mark the variable to be modified only if 
`CheckForModifiableLvalue` returns true ?

2. I think that you need to handle way more than just member expressions. For 
example are you handling `(x, y)` (comma operator) ? But hard-coding every 
cases here is probably not ideal. It would be nice if there was already some 
code somewhere that could help you do this.

3. I believe that a `MemberExpr` has always a base. Similarly `DeclRefExpr`s 
always have a referenced declaration (so you can omit the `if`).


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

https://reviews.llvm.org/D58035



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

[PATCH] D58035: [clang/DIVar] Emit flag for params that have unchanged values

[Djordje Todorovic via Phabricator via cfe-commits]

djtodoro updated this revision to Diff 187931.
djtodoro added a comment.

- Add a field in `ParmVarDecl` instead of `VarDecl`
- Use a bit in `ParmVarDeclBitfields` to indicate parameter modification
- Add support for the bit in  `ASTReaderDecl.cpp` / `ASTWriterDecl.cpp`
- Add test case for templates


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

https://reviews.llvm.org/D58035

Files:
  include/clang/AST/Decl.h
  lib/CodeGen/CGDebugInfo.cpp
  lib/Sema/SemaExpr.cpp
  lib/Serialization/ASTReaderDecl.cpp
  lib/Serialization/ASTWriterDecl.cpp
  test/CodeGen/dbginfo-var-change-templates.cpp
  test/CodeGen/debug-info-varchange.c

Index: test/CodeGen/debug-info-varchange.c
===
--- /dev/null
+++ test/CodeGen/debug-info-varchange.c
@@ -0,0 +1,35 @@
+// RUN: %clang -femit-param-entry-values -emit-llvm -S -g %s -o - | FileCheck %s
+
+// CHECK: !DILocalVariable(name: "a", arg: 1, scope: {{.*}}, file: {{.*}}, line: {{.*}}, type: {{.*}}, flags: DIFlagArgumentNotModified)
+// CHECK: !DILocalVariable(name: "b", arg: 2, scope: {{.*}}, file: {{.*}}, line: {{.*}}, type: {{.*}})
+// CHECK: !DILocalVariable(name: "test_s", arg: 3, scope: {{.*}}, file: {{.*}}, line: {{.*}}, type: {{.*}}, flags: DIFlagArgumentNotModified)
+// CHECK: !DILocalVariable(name: "test_s2", arg: 4, scope: {{.*}}, file: {{.*}}, line: {{.*}}, type: {{.*}})
+// CHECK: !DILocalVariable(name: "x", scope: {{.*}}, file: {{.*}}, line: {{.*}}, type: {{.*}})
+// CHECK: !DILocalVariable(name: "y", scope: {{.*}}, file: {{.*}}, line: {{.*}}, type: {{.*}})
+
+typedef struct s {
+  int m;
+  int n;
+}S;
+
+void foo (int a, int b,
+  S test_s, S test_s2)
+{
+  b++;
+  b = a + 1;
+  if (b>4)
+test_s2.m = 434;
+}
+
+int main()
+{
+  S test_s = {4, 5};
+
+  int x = 5;
+  int y = 6;
+
+  foo(x , y, test_s, test_s);
+
+  return 0;
+}
+
Index: test/CodeGen/dbginfo-var-change-templates.cpp
===
--- /dev/null
+++ test/CodeGen/dbginfo-var-change-templates.cpp
@@ -0,0 +1,24 @@
+// RUN: %clang -femit-param-entry-values -emit-llvm -S -g %s -o - | FileCheck %s
+// CHECK: !DILocalVariable(name: "a", arg: 1, scope: {{.*}}, file: {{.*}}, line: {{.*}}, type: {{.*}})
+// CHECK: !DILocalVariable(name: "b", arg: 2, scope: {{.*}}, file: {{.*}}, line: {{.*}}, type: {{.*}}, flags: DIFlagArgumentNotModified)
+
+template 
+__attribute__((noinline))
+T GetMin (T a, T b) {
+  T result;
+  ++a;
+  result = (a < b) ? a : b;
+  return result;
+}
+
+int baa () {
+  int i=5, j=6, k;
+  k=GetMin(i,j);
+
+  if (i == k)
+++k;
+  else
+--k;
+
+  return k;
+}
Index: lib/Serialization/ASTWriterDecl.cpp
===
--- lib/Serialization/ASTWriterDecl.cpp
+++ lib/Serialization/ASTWriterDecl.cpp
@@ -1033,6 +1033,7 @@
   Record.push_back(D->hasUninstantiatedDefaultArg());
   if (D->hasUninstantiatedDefaultArg())
 Record.AddStmt(D->getUninstantiatedDefaultArg());
+  Record.push_back(D->isArgumentKnownToBeModified());
   Code = serialization::DECL_PARM_VAR;
 
   assert(!D->isARCPseudoStrong()); // can be true of ImplicitParamDecl
@@ -1046,6 +1047,7 @@
   !D->isImplicit() &&
   !D->isUsed(false) &&
   !D->isInvalidDecl() &&
+  !D->isArgumentKnownToBeModified() &&
   !D->isReferenced() &&
   D->getAccess() == AS_none &&
   !D->isModulePrivate() &&
@@ -2011,6 +2013,7 @@
   Abv->Add(BitCodeAbbrevOp(0));   // KNRPromoted
   Abv->Add(BitCodeAbbrevOp(0));   // HasInheritedDefaultArg
   Abv->Add(BitCodeAbbrevOp(0));   // HasUninstantiatedDefaultArg
+  Abv->Add(BitCodeAbbrevOp(0));   // IsArgumentKnownToBeModified
   // Type Source Info
   Abv->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::Array));
   Abv->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::VBR, 6)); // TypeLoc
Index: lib/Serialization/ASTReaderDecl.cpp
===
--- lib/Serialization/ASTReaderDecl.cpp
+++ lib/Serialization/ASTReaderDecl.cpp
@@ -1441,7 +1441,7 @@
   PD->ParmVarDeclBits.HasInheritedDefaultArg = Record.readInt();
   if (Record.readInt()) // hasUninstantiatedDefaultArg.
 PD->setUninstantiatedDefaultArg(Record.readExpr());
-
+  PD->ParmVarDeclBits.IsArgumentKnownToBeModified = Record.readInt();
   // FIXME: If this is a redeclaration of a function from another module, handle
   // inheritance of default arguments.
 }
Index: lib/Sema/SemaExpr.cpp
===
--- lib/Sema/SemaExpr.cpp
+++ lib/Sema/SemaExpr.cpp
@@ -16,6 +16,7 @@
 #include "clang/AST/ASTLambda.h"
 #include "clang/AST/ASTMutationListener.h"
 #include "clang/AST/CXXInheritance.h"
+#include "clang/AST/Decl.h"
 #include "clang/AST/DeclObjC.h"
 #include "clang/AST/DeclTemplate.h"
 #include "clang/AST/EvaluatedExprVisitor.h"
@@ -11276,6 +11277,15 @@
 

[PATCH] D58035: [clang/DIVar] Emit flag for params that have unchanged values

[Djordje Todorovic via Phabricator via cfe-commits]

djtodoro marked an inline comment as done.
djtodoro added a comment.

@riccibruno Thanks for your comments!

> Oh and I think that you will also have to update the serialization 
> code/de-serialization code in ASTReaderDecl.cpp / ASTWriterDecl.cpp. You 
> might also have to update TreeTransform but I am less familiar with this.

I've added `ASTReaderDecl.cpp` / `ASTWriterDecl.cpp`, but looking at 
`TreeTransform ` I'm not sure if there is a place for adding something 
regarding this.

> I am wondering about the semantics of this bit. I don't think that you can 
> know for sure within clang whether a variable has been modified. The best you 
> can do is know for sure that some variable has been modified, but I don't 
> think you can prove that it has not been modified.



> Consequently I am wondering if you should change the name of this flag to 
> something like IsArgumentKnownToBeModified.



> Orthogonal to this you need to also add tests for templates.

I agree! You are right, good idea!


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

https://reviews.llvm.org/D58035



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

[PATCH] D58035: [clang/DIVar] Emit flag for params that have unchanged values

[Djordje Todorovic via Phabricator via cfe-commits]

djtodoro marked 2 inline comments as done.
djtodoro added a comment.

> I was under the impression that space inside VarDecl was quite constrained. 
> Pardon the likely naive question, but: is there any way to make the 
> representation more compact (maybe sneak a bit into ParmVarDeclBitfields)?

@vsk  Thanks for the comment! Sure, it is better idea. Initially, we thought 
this could be used even for local variables, but since we are using it only for 
parameters it makes more sense.




Comment at: lib/Sema/SemaExpr.cpp:11282
+static void EmitArgumentsValueModification(Expr *E) {
+  if (const DeclRefExpr *LHSDeclRef =
+  dyn_cast(E->IgnoreParenCasts()))

probinson wrote:
> Does this fit on one line if you use `const auto *LHSDeclRef ...`? Given you 
> have the dyn_cast on the RHS there's no real need to give the type name twice.
>Does this fit on one line if you use const auto *LHSDeclRef ...? Given you 
>have the dyn_cast on the RHS there's no real need to give the type name twice.

@probinson Thanks for the comment! Sure, it will be refactored.


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

https://reviews.llvm.org/D58035



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

[PATCH] D58035: [clang/DIVar] Emit flag for params that have unchanged values

[Bruno Ricci via Phabricator via cfe-commits]

riccibruno added a comment.

I am wondering about the semantics of this bit. I don't think that you can know 
for sure within clang whether a variable has been modified. The best you can do 
is know for sure that some variable has been modified, but I don't think you 
can prove that it has not been modified.

Consequently I am wondering if you should change the name of this flag to 
something like `IsArgumentKnownToBeModified`.

Orthogonal to this you need to also add tests for templates.


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

https://reviews.llvm.org/D58035



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

[PATCH] D58035: [clang/DIVar] Emit flag for params that have unchanged values

[Bruno Ricci via Phabricator via cfe-commits]

riccibruno added a comment.

Oh and I think that you will also have to update the serialization 
code/de-serialization code in `ASTReaderDecl.cpp` / `ASTWriterDecl.cpp`. You 
might also have to update `TreeTransform` but I am less familiar with this.


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

https://reviews.llvm.org/D58035



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

[PATCH] D58035: [clang/DIVar] Emit flag for params that have unchanged values

[Bruno Ricci via Phabricator via cfe-commits]

riccibruno added a comment.

If this bit is relevant to function parameters, why is `getIsArgumentModified` 
in `VarDecl` and not in `ParamVarDecl` ? What you can do is move the relevant 
methods to `ParamVarDecl`, and stash the bit in `ParmVarDeclBitfields`.




Comment at: include/clang/AST/Decl.h:843
 
+  mutable bool IsArgumentModified = false;
+

Yeah, please don't waste a whole pointer just to store a bit.


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

https://reviews.llvm.org/D58035



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

[PATCH] D58035: [clang/DIVar] Emit flag for params that have unchanged values

[Vedant Kumar via Phabricator via cfe-commits]

vsk added reviewers: bricci, rsmith.
vsk added a comment.

+ rsmith, + bricci for review.

I was under the impression that space inside VarDecl was quite constrained. 
Pardon the likely naive question, but: is there any way to make the 
representation more compact (maybe sneak a bit into ParmVarDeclBitfields)?


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

https://reviews.llvm.org/D58035



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

[PATCH] D58035: [clang/DIVar] Emit flag for params that have unchanged values

[Paul Robinson via Phabricator via cfe-commits]

probinson added inline comments.



Comment at: include/clang/AST/Decl.h:1482
 
+  bool getIsArgumentModified() const {
+return IsArgumentModified;

There should be a comment here.
The style in this class appears to omit the 'get' word from the name of the 
getter, so `isArgumentModified`  for the method name would look more consistent.



Comment at: lib/Sema/SemaExpr.cpp:11282
+static void EmitArgumentsValueModification(Expr *E) {
+  if (const DeclRefExpr *LHSDeclRef =
+  dyn_cast(E->IgnoreParenCasts()))

Does this fit on one line if you use `const auto *LHSDeclRef ...`? Given you 
have the dyn_cast on the RHS there's no real need to give the type name twice.


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

https://reviews.llvm.org/D58035



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

[PATCH] D58035: [clang/DIVar] Emit flag for params that have unchanged values

[Djordje Todorovic via Phabricator via cfe-commits]

djtodoro updated this revision to Diff 186611.
djtodoro added a comment.

- Rename: `VariableNotChanged `===> `ArgumentNotModified`
- Refactor a test case


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

https://reviews.llvm.org/D58035

Files:
  include/clang/AST/Decl.h
  lib/CodeGen/CGDebugInfo.cpp
  lib/Sema/SemaExpr.cpp
  test/CodeGen/debug-info-varchange.c

Index: test/CodeGen/debug-info-varchange.c
===
--- /dev/null
+++ test/CodeGen/debug-info-varchange.c
@@ -0,0 +1,35 @@
+// RUN: %clang -femit-param-entry-values -emit-llvm -S -g %s -o - | FileCheck %s
+
+// CHECK: !DILocalVariable(name: "a", arg: 1, scope: {{.*}}, file: {{.*}}, line: {{.*}}, type: {{.*}}, flags: DIFlagArgumentNotModified)
+// CHECK: !DILocalVariable(name: "b", arg: 2, scope: {{.*}}, file: {{.*}}, line: {{.*}}, type: {{.*}})
+// CHECK: !DILocalVariable(name: "test_s", arg: 3, scope: {{.*}}, file: {{.*}}, line: {{.*}}, type: {{.*}}, flags: DIFlagArgumentNotModified)
+// CHECK: !DILocalVariable(name: "test_s2", arg: 4, scope: {{.*}}, file: {{.*}}, line: {{.*}}, type: {{.*}})
+// CHECK: !DILocalVariable(name: "x", scope: {{.*}}, file: {{.*}}, line: {{.*}}, type: {{.*}})
+// CHECK: !DILocalVariable(name: "y", scope: {{.*}}, file: {{.*}}, line: {{.*}}, type: {{.*}})
+
+typedef struct s {
+  int m;
+  int n;
+}S;
+
+void foo (int a, int b,
+  S test_s, S test_s2)
+{
+  b++;
+  b = a + 1;
+  if (b>4)
+test_s2.m = 434;
+}
+
+int main()
+{
+  S test_s = {4, 5};
+
+  int x = 5;
+  int y = 6;
+
+  foo(x , y, test_s, test_s);
+
+  return 0;
+}
+
Index: lib/Sema/SemaExpr.cpp
===
--- lib/Sema/SemaExpr.cpp
+++ lib/Sema/SemaExpr.cpp
@@ -16,6 +16,7 @@
 #include "clang/AST/ASTLambda.h"
 #include "clang/AST/ASTMutationListener.h"
 #include "clang/AST/CXXInheritance.h"
+#include "clang/AST/Decl.h"
 #include "clang/AST/DeclObjC.h"
 #include "clang/AST/DeclTemplate.h"
 #include "clang/AST/EvaluatedExprVisitor.h"
@@ -11276,6 +11277,19 @@
 DiagnoseConstAssignment(S, E, Loc);
 }
 
+/// Argument's value might be modified, so update the info.
+static void EmitArgumentsValueModification(Expr *E) {
+  if (const DeclRefExpr *LHSDeclRef =
+  dyn_cast(E->IgnoreParenCasts()))
+if (const ValueDecl *Decl = LHSDeclRef->getDecl())
+  if (const VarDecl *VD = dyn_cast(Decl)) {
+if (!isa(VD))
+  return;
+if (!VD->getIsArgumentModified())
+  VD->setIsArgumentModified();
+  }
+}
+
 /// CheckForModifiableLvalue - Verify that E is a modifiable lvalue.  If not,
 /// emit an error and return true.  If so, return false.
 static bool CheckForModifiableLvalue(Expr *E, SourceLocation Loc, Sema ) {
@@ -11283,6 +11297,12 @@
 
   S.CheckShadowingDeclModification(E, Loc);
 
+  if (MemberExpr *ME = dyn_cast(E)) {
+if (Expr *BaseExpr = ME->getBase())
+  EmitArgumentsValueModification(BaseExpr);
+  } else
+EmitArgumentsValueModification(E);
+
   SourceLocation OrigLoc = Loc;
   Expr::isModifiableLvalueResult IsLV = E->isModifiableLvalue(S.Context,
   );
Index: lib/CodeGen/CGDebugInfo.cpp
===
--- lib/CodeGen/CGDebugInfo.cpp
+++ lib/CodeGen/CGDebugInfo.cpp
@@ -3778,6 +3778,11 @@
   if (VD->isImplicit())
 Flags |= llvm::DINode::FlagArtificial;
 
+  auto  = CGM.getCodeGenOpts();
+  if (CGOpts.EnableParamEntryValues && !VD->getIsArgumentModified())
+Flags |= llvm::DINode::FlagArgumentNotModified;
+
+
   auto Align = getDeclAlignIfRequired(VD, CGM.getContext());
 
   unsigned AddressSpace = CGM.getContext().getTargetAddressSpace(VD->getType());
Index: include/clang/AST/Decl.h
===
--- include/clang/AST/Decl.h
+++ include/clang/AST/Decl.h
@@ -840,6 +840,8 @@
   /// It is illegal to call this function with SC == None.
   static const char *getStorageClassSpecifierString(StorageClass SC);
 
+  mutable bool IsArgumentModified = false;
+
 protected:
   // A pointer union of Stmt * and EvaluatedStmt *. When an EvaluatedStmt, we
   // have allocated the auxiliary struct of information there.
@@ -1477,6 +1479,13 @@
   /// Do we need to emit an exit-time destructor for this variable?
   bool isNoDestroy(const ASTContext &) const;
 
+  bool getIsArgumentModified() const {
+return IsArgumentModified;
+  }
+  void setIsArgumentModified() const {
+IsArgumentModified = true;
+  }
+
   // Implement isa/cast/dyncast/etc.
   static bool classof(const Decl *D) { return classofKind(D->getKind()); }
   static bool classofKind(Kind K) { return K >= firstVar && K <= lastVar; }
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits