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

[Diego Treviño via Phabricator via llvm-commits]

diegotf 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
----------------
dblaikie wrote:
> 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)
Good catch! I hadn't thought of that.


================
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,
----------------
dblaikie wrote:
> 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?
I've refactored the Generic Delta pass in order to avoid this problem. I believe it's also simpler and easier to read.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64176