[PATCH] D64176: [Bugpoint redesign] Added Pass to Remove Global Variables

[David Blaikie via Phabricator via llvm-commits]

dblaikie added inline comments.


================
Comment at: llvm/test/Reduce/remove-global-vars.ll:8-14
+; CHECK-NOT: @uninteresting1
+ at uninteresting1 = global i32 0, align 4
+; CHECK: @interesting
+ at interesting = global i32 5, align 4
+; CHECK-NOT: @uninteresting2
+ at uninteresting2 = global i32 25, align 4
+; CHECK-NOT: @uninteresting3
----------------
Might be stronger to:

CHECK-NOT: "global" on either side of the "CHECK: @interesting = global"

That way it'd be resilient to any reordering, for instance (where you could silently pass the test if LLVM IR happened to print 2, 3, unnumbered, 1)


================
Comment at: llvm/tools/llvm-reduce/deltas/RemoveGlobalVars.cpp:53
+        ++I;
+    ++GVCount;
+    }
----------------
Is the indentation off here? (clang-format can be handy to clean up stuff like this)


================
Comment at: llvm/tools/llvm-reduce/deltas/RemoveGlobalVars.h:23-31
+class RemoveGlobalVars {
+public:
+  /// Outputs the number of initialized GVs in the given Module.
+  static int getTargetCount(Module *Program);
+  /// Clones module and returns one without non-chunk GVs
+  static std::unique_ptr<Module> extractChunksFromModule(
+      std::vector<Chunk> ChunksToKeep,
----------------
Classes to group static functions are somewhat to be avoided, I think - should this be a helper namespace, or maybe leaving the functions in the 'llvm' namespace while making the names more specific?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64176