[PATCH] D150221: Add option -fkeep-static-variables to emit all static variables

qianzhen updated this revision to Diff 537175. qianzhen added a comment. Update to address review comments Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D150221/new/ https://reviews.llvm.org/D150221 Files:

[PATCH] D150221: Add option -fkeep-static-variables to emit all static variables

hubert.reinterpretcast added inline comments. Comment at: clang/include/clang/Basic/CodeGenOptions.def:475 +/// Whether to emit all variables that have a persisent storage duration, +/// including global, static and thread local variables. Minor nit: Typo.

[PATCH] D150221: Add option -fkeep-static-variables to emit all static variables

qianzhen added a comment. @erichkeane Thanks for the comments! > How does this work with function-local statics? This is a valid case. The function-local statics should be kept; the missing implementation is added now. > What does the behavior look like in C? The C behavior should be the

[PATCH] D150221: Add option -fkeep-static-variables to emit all static variables

qianzhen updated this revision to Diff 529601. qianzhen added a comment. More examples have been identified for the adaptation of IBM XL compiler's -qstatsym option for the hot patch use case, which was mentioned previously. Therefore, this option is extended to cover the following cases. 1.

[PATCH] D150221: Add option -fkeep-static-variables to emit all static variables

erichkeane added a comment. This seems a little short on tests as well, are there any semantic implications here that should be validated? How does this work with function-local statics? What does the behavior look like in C? Else, I think @rjmccall felt the strongest about the

[PATCH] D150221: Add option -fkeep-static-variables to emit all static variables

hubert.reinterpretcast added inline comments. Comment at: clang/test/CodeGen/keep-static-variables.cpp:1 +// RUN: %clang_cc1 -fkeep-static-variables -emit-llvm %s -o - -triple=x86_64-unknown-linux-gnu | FileCheck %s --check-prefixes=CHECK,CHECK-ELF +// RUN: %clang_cc1

[PATCH] D150221: Add option -fkeep-static-variables to emit all static variables

hubert.reinterpretcast added inline comments. Comment at: clang/lib/CodeGen/CodeGenModule.cpp:2201-2210 + if ((CodeGenOpts.KeepStaticConsts || CodeGenOpts.KeepStaticVariables) && D && + isa(D)) { const auto *VD = cast(D); -if (VD->getType().isConstQualified() && -

[PATCH] D150221: Add option -fkeep-static-variables to emit all static variables

qianzhen updated this revision to Diff 526090. qianzhen added a comment. Update the option text to be more descriptive Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D150221/new/ https://reviews.llvm.org/D150221 Files:

[PATCH] D150221: Add option -fkeep-static-variables to emit all static variables

efriedma added a comment. I think I'm okay with the semantics as-is. Comment at: clang/include/clang/Driver/Options.td:1703 + PosFlag, NegFlag, + BothFlags<[NoXarchOption], " static variables if unused">>; defm fixed_point : BoolFOption<"fixed-point", Can

[PATCH] D150221: Add option -fkeep-static-variables to emit all static variables

qianzhen added a comment. Gentle ping. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D150221/new/ https://reviews.llvm.org/D150221 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D150221: Add option -fkeep-static-variables to emit all static variables

qianzhen updated this revision to Diff 523546. qianzhen added a comment. Refactor as suggested Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D150221/new/ https://reviews.llvm.org/D150221 Files: clang/include/clang/Basic/CodeGenOptions.def

[PATCH] D150221: Add option -fkeep-static-variables to emit all static variables

hubert.reinterpretcast added a comment. In D150221#4351249 , @rjmccall wrote: > Force-emitting every `static` in the translation unit can be very expensive; > there are a lot of headers that declare all their constants as `static > const`. And

[PATCH] D150221: Add option -fkeep-static-variables to emit all static variables

rjmccall added a comment. The hot-patch use case actually shouldn't care if the compiler/linker throws away unused things because it can't possibly affect how the patch interoperates with existing code. This does rely on the unanalyzable-use part of the semantics, though, to stop the compiler

[PATCH] D150221: Add option -fkeep-static-variables to emit all static variables

hubert.reinterpretcast added a comment. In D150221#4350761 , @rjmccall wrote: > I'm not sure if it's better to represent that by using > `__attribute__((used))` on every global variable or by setting something more > globally in the module. The latter

[PATCH] D150221: Add option -fkeep-static-variables to emit all static variables

rjmccall added a comment. Hmm. Preserving unused global variables in case a hot patch might use them doesn't seem very compelling to me — hot patches need to be able to introduce globals of their own anyway. What I find more convincing is related to the *other* meaning of

[PATCH] D150221: Add option -fkeep-static-variables to emit all static variables

hubert.reinterpretcast added a comment. In D150221#4347840 , @efriedma wrote: >> This is an adaptation of the IBM XL compiler's -qstatsym option, which is >> meant to generate symbol table entries for static variables. An artifact of >> that compiler

[PATCH] D150221: Add option -fkeep-static-variables to emit all static variables

efriedma added a comment. > This is an adaptation of the IBM XL compiler's -qstatsym option, which is > meant to generate symbol table entries for static variables. An artifact of > that compiler is that static variables are often not discarded even when > unused. Oh, I see; the compiler

[PATCH] D150221: Add option -fkeep-static-variables to emit all static variables

hubert.reinterpretcast added a comment. In D150221#4343546 , @efriedma wrote: > It's not unprecedented to add flags to copy the behavior of other compilers, > to make porting easier, especially when it doesn't place much burden on > compiler

[PATCH] D150221: Add option -fkeep-static-variables to emit all static variables

efriedma added a comment. It's not unprecedented to add flags to copy the behavior of other compilers, to make porting easier, especially when it doesn't place much burden on compiler maintainers. But what compiler preserves the names/values of static variables by default? It's not the sort

[PATCH] D150221: Add option -fkeep-static-variables to emit all static variables

aaron.ballman added reviewers: rjmccall, efriedma. aaron.ballman added subscribers: efriedma, rjmccall. aaron.ballman added a comment. In D150221#4338500 , @qianzhen wrote: > This is useful in keeping the static variables in a patchable function >

[PATCH] D150221: Add option -fkeep-static-variables to emit all static variables

qianzhen added a comment. This is useful in keeping the static variables in a patchable function (https://clang.llvm.org/docs/AttributeReference.html#patchable-function-entry), so that they can be directly addressed by a hot patch when the optimization to merge them is enabled

[PATCH] D150221: Add option -fkeep-static-variables to emit all static variables

hubert.reinterpretcast added a comment. In D150221#4332142 , @erichkeane wrote: >> This is intended to prevent "excessive transformation" to enable migration >> of existing applications (using a non-Clang compiler) where users further >> manipulate

[PATCH] D150221: Add option -fkeep-static-variables to emit all static variables

erichkeane added a subscriber: aaron.ballman. erichkeane added a comment. In D150221#4330534 , @hubert.reinterpretcast wrote: > In D150221#4330504 , @MaskRay wrote: > >> Can you give a more compelling reason

[PATCH] D150221: Add option -fkeep-static-variables to emit all static variables

cebowleratibm added inline comments. Comment at: clang/lib/CodeGen/CodeGenModule.cpp:2207 +addUsedOrCompilerUsedGlobal(GV); + else if (CodeGenOpts.KeepStaticConsts && VD->getType().isConstQualified()) +addUsedOrCompilerUsedGlobal(GV); why

[PATCH] D150221: Add option -fkeep-static-variables to emit all static variables

hubert.reinterpretcast added a comment. In D150221#4330504 , @MaskRay wrote: > Can you give a more compelling reason that this option is needed? This is intended to prevent "excessive transformation" to enable migration of existing applications (using

[PATCH] D150221: Add option -fkeep-static-variables to emit all static variables

MaskRay added a comment. GCC has -fkeep-static-consts (which Clang supports) but not -fkeep-static-variables. > This could be useful in cases where the presence of all static variables as > symbols in the object file are required. Can you give a more compelling reason that this option is

[PATCH] D150221: Add option -fkeep-static-variables to emit all static variables

qianzhen created this revision. Herald added a project: All. qianzhen requested review of this revision. Herald added subscribers: cfe-commits, MaskRay. Herald added a project: clang. This patch adds a new option -fkeep-static-variables to emit all static variables if required. This could be