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

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 com

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

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-modific

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

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/Code

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

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 ___

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

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: li

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

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 li

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

aprantl added inline comments. Comment at: lib/CodeGen/CGDebugInfo.cpp:4537 + CGM.getLangOpts().Optimize) { +for (auto &SP : DeclCache) { + auto *D = SP.first; djtodoro wrote: > aprantl wrote: > > djtodoro wrote: > > > aprantl wrote: > > > > Just lo

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

djtodoro marked 3 inline comments as done. djtodoro added inline comments. Comment at: lib/CodeGen/CGDebugInfo.cpp:4537 + CGM.getLangOpts().Optimize) { +for (auto &SP : DeclCache) { + auto *D = SP.first; aprantl wrote: > djtodoro wrote: > > aprantl

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

aprantl added inline comments. Comment at: lib/CodeGen/CGDebugInfo.cpp:4537 + CGM.getLangOpts().Optimize) { +for (auto &SP : DeclCache) { + auto *D = SP.first; djtodoro wrote: > aprantl wrote: > > Just looking at the type declarations in CGDebugInfo

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

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/C

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

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

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

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)` che

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

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

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

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 Ind

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

djtodoro marked 3 inline comments as done. djtodoro added inline comments. Comment at: lib/CodeGen/CGDebugInfo.cpp:4537 + CGM.getLangOpts().Optimize) { +for (auto &SP : DeclCache) { + auto *D = SP.first; aprantl wrote: > Just looking at the type dec

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

aprantl added inline comments. Comment at: lib/CodeGen/CGDebugInfo.cpp:4537 + CGM.getLangOpts().Optimize) { +for (auto &SP : DeclCache) { + auto *D = SP.first; Just looking at the type declarations in CGDebugInfo.h: Why not iterate over the `SPCach

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

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/CG

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

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

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

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 mo

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

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/cl

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

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 >>

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

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

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

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 > `CheckForModifiabl

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

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

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

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 template

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

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 bu

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

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)? @

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

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

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

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.ll

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

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

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

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 ParmVarDec

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

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 gette

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

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/Cod