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

[Sergei Larin via llvm-commits]

slarin added a comment.

Peter,

  You are right in general - we have seen this bias. To address the issue, we re-globalize things that were internalized for LTO leaving only _real_ dependencies. If that step is interesting for anyone - we can definitely upstream it... but it will not go all the way. Constant merging sometimes introduces additional dependencies that might be counterproductive for splitting, and we are currently looking at our options to address that.

Nevertheless, without this patch the whole (split) feature is kind of useless for us, and I will try to explain why when I answer to Mehdi's question...


================
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,
----------------
pcc wrote:
> 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.
I can see it both ways. In my tree I have one more change that is not (yet) reflected in this patch- partially I did not want to change several things at once... for that case having custom sorting function as argument makes a lot of sense. 

On the other hand, I wanted to preserve as much from the original implementation as possible since i do not have a good visibility into all possible use cases.

I guess, if we can simply add "split mode" flag to splitCodeGen it might do the trick in fewer lines of code. Your are the owner of this code - in some way it is your call...

================
Comment at: include/llvm/CodeGen/ParallelCG.h:41
@@ +40,3 @@
+        SplitFunction,
+    Reloc::Model RM = Reloc::Default, CodeModel::Model CM = CodeModel::Default,
+    CodeGenOpt::Level OL = CodeGenOpt::Default,
----------------
joker.eph wrote:
> The added argument would probably benefit some documentation.
Do you mean in comments? Sure - based on the final form-factor we would want to use.

================
Comment at: lib/Transforms/Utils/SplitModule.cpp:47
@@ +46,3 @@
+
+static void addGlobalValueUser(ClusterMapType &GVtoClusterMap,
+                               const GlobalValue *GV,
----------------
pcc wrote:
> I believe that what this function does is already implemented by `llvm::EquivalenceClasses`.
Maybe... I need to look if it can be used for this - one question would be the use of std::shared_ptr in this case - memory and time is an issue for us. This code is highly stressed.

If you are OK with the general concept - I can always improve it with additional patches - I am planning to stay here for a while :)

================
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);
+    }
+  }
+}
----------------
pcc wrote:
> 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.
Yes, I looked into that, and it seemed to me that doing it like this was marginally faster. Is your concern with code clarity or implementation efficiency?

================
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;
----------------
pcc wrote:
> You can simplify this code by removing this line.
I am not sure about that - I will let Tobias to comment on this...

================
Comment at: lib/Transforms/Utils/SplitModule.cpp:304
@@ +303,3 @@
+    if (I != 0)
+      MPart->setModuleInlineAsm("");
+    ModuleCallback(std::move(MPart));
----------------
pcc wrote:
> 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.)
Generally agree... we can do it in following patches...


http://reviews.llvm.org/D16124