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

[Hubert Tong via Phabricator via cfe-commits]

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.


================
Comment at: clang/include/clang/Driver/Options.td:1700
   BothFlags<[NoXarchOption], " static const variables if unused">>;
+defm keep_persistent_storage_variables : BoolFOption<"keep-persistent-storage-variables",
+  CodeGenOpts<"KeepPersistentStorageVariables">, DefaultFalse,
----------------
Please update the patch title and description to match.


================
Comment at: clang/test/CodeGen/keep-persistent-storage-variables.cpp:18-19
+// CHECK: @_ZN2ST2s6E = global i32 7, align 4
+// CHECK-ELF: @llvm.compiler.used = appending global [14 x ptr] [ptr @_ZL2g1, ptr @_ZL2g2, ptr @_ZL2g3, ptr @_ZL2g4, ptr @tl1, ptr @tl2, ptr @_ZL3tl3, ptr @_ZL3tl4, ptr @g5, ptr @g6, ptr @_ZZ5test3vE2s3, ptr @_ZN12_GLOBAL__N_12s4E, ptr @_ZZ5test5vE3tl5, ptr @_ZN2ST2s6E], section "llvm.metadata"
+// CHECK-NON-ELF: @llvm.used = appending global [14 x ptr] [ptr @_ZL2g1, ptr @_ZL2g2, ptr @_ZL2g3, ptr @_ZL2g4, ptr @tl1, ptr @tl2, ptr @_ZL3tl3, ptr @_ZL3tl4, ptr @g5, ptr @g6, ptr @_ZZ5test3vE2s3, ptr @_ZN12_GLOBAL__N_12s4E, ptr @_ZZ5test5vE3tl5, ptr @_ZN2ST2s6E], section "llvm.metadata"
+
----------------
If we don't care which of `llvm.compiler.used` or `llvm.used` is used (no pun intended), then maybe we can use a regex? For example, `@llvm{{(\.compiler)?}}.used`.


================
Comment at: clang/test/CodeGen/keep-persistent-storage-variables.cpp:32-39
+int test1() {
+  g1 = 3;
+  return g1;
+}
+
+int test2() {
+  return g2;
----------------
Why add functions that use `g1` and `g2`? Is there some reason to suspect that using the variables will cause them not to be emitted as explicitly used?

If removing these functions, please also remove `g3` and `g4` as redundant.


================
Comment at: clang/test/CodeGen/keep-persistent-storage-variables.cpp:50
+}
+void *test4() { return &s4; }
+
----------------
Same comment re: why add a function?


================
Comment at: clang/test/Driver/fkeep-persistent-storage-variables.c:1
+// RUN: %clang -fkeep-persistent-storage-variables -c %s -### 2>&1 | FileCheck %s
+
----------------
Please add test for `-fkeep-persistent-storage-variables -fno-keep-persistent-storage-variables`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150221