[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.

> Constant merging sometimes introduces additional dependencies that might be counterproductive for splitting, and we are currently looking at our options to address that.


How bad is the problem? I think that as long as your re-globalization is conservatively correct (i.e. does not pollute the global namespace) it would still be a useful contribution.

> Nevertheless, without this patch the whole (split) feature is kind of useless for us


I can understand that, but to be frank, your patch isn't very useful for upstream without the re-globalization step.


================
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,
----------------
slarin wrote:
> 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...
> 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.

I'd like to understand why.

> 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.

The reason for externalizing everything was mostly "implementation simplicity at the cost of some correctness". If we can fix the correctness problem then I'd rather not have two implementations.

================
Comment at: lib/Transforms/Utils/SplitModule.cpp:47
@@ +46,3 @@
+
+static void addGlobalValueUser(ClusterMapType &GVtoClusterMap,
+                               const GlobalValue *GV,
----------------
slarin wrote:
> 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 :)
It certainly seems that `EquivalenceClasses` would be more efficient than your custom implementation here, I'd also strongly prefer not to have a second implementation of equivalence classes in the tree if at all possible.

================
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);
+    }
+  }
+}
----------------
slarin wrote:
> 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?
Mostly code clarity. If it's faster, that's all the better.


http://reviews.llvm.org/D16124