[PATCH] D16124: Add to the split module utility an SCC based method which allows not to globalize any local variables.

[Peter Collingbourne via llvm-commits]

pcc added a comment.

As far as I can tell, it seems that in a typical LTO scenario with internalization, this partitioning would be strongly biased towards a single partition containing most of the code. Do you have a plan to address this?

I also have some lower level comments which will become more relevant if you can address the above.


================
Comment at: include/llvm/CodeGen/ParallelCG.h:37-40
@@ +36,6 @@
+    StringRef Features, const TargetOptions &Options,
+    std::function<
+        void(std::unique_ptr<Module> M, unsigned N,
+             std::function<void(std::unique_ptr<Module> MPart)> ModuleCallback)>
+        SplitFunction,
+    Reloc::Model RM = Reloc::Default, CodeModel::Model CM = CodeModel::Default,
----------------
I don't think we need this extra flexibility. If this new way is better I think we should replace the existing `SplitModule` implementation with it.

================
Comment at: lib/Transforms/Utils/SplitModule.cpp:47
@@ +46,3 @@
+
+static void addGlobalValueUser(ClusterMapType &GVtoClusterMap,
+                               const GlobalValue *GV,
----------------
I believe that what this function does is already implemented by `llvm::EquivalenceClasses`.

================
Comment at: lib/Transforms/Utils/SplitModule.cpp:113-123
@@ +112,13 @@
+
+  for (auto &&CM : make_range(ComdatMembers.equal_range(C))) {
+    if (CM.second == GV)
+      continue;
+    DEBUG(dbgs() << "In the same comdat: (" << GV->getName() << ") and ("
+                 << CM.second->getName() << ")\n");
+    if (GV->hasLocalLinkage()) {
+      addGlobalValueUser(GVtoClusterMap, GV, CM.second);
+    } else if (CM.second->hasLocalLinkage()) {
+      addGlobalValueUser(GVtoClusterMap, CM.second, GV);
+    }
+  }
+}
----------------
I think you can do all this stuff in one go with a loop that iterates over the whole map after visiting all the globals.

================
Comment at: lib/Transforms/Utils/SplitModule.cpp:161
@@ +160,3 @@
+      // For each constant that is not a GV (a pure const):
+      if (isa<Constant>(U) && !isa<GlobalValue>(U)) {
+        SmallVector<const User *, 8> Worklist;
----------------
You can simplify this code by removing this line.

================
Comment at: lib/Transforms/Utils/SplitModule.cpp:172-175
@@ +171,6 @@
+        }
+      } else {
+        // User is an instruction, alias or GlobalValue.
+        addNonConstUser(GVtoClusterMap, &GV, U);
+      }
+    }
----------------
Same here

================
Comment at: lib/Transforms/Utils/SplitModule.cpp:304
@@ +303,3 @@
+    if (I != 0)
+      MPart->setModuleInlineAsm("");
+    ModuleCallback(std::move(MPart));
----------------
Should we do something better with module inline asm? Maybe add referenced globals to an equivalence class? (Probably doesn't need to happen in the initial version though.)


http://reviews.llvm.org/D16124