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

Peter,

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

It is entirely property of your code. If you do have a lot of reusable constants in the LTO module, const merging acts as a spaghetti explosion connecting otherwise unrelated SCC. We are considering two ways for dealing with it - do not merge heuristic (likely to be slow), or re-split some constants smartly - this could be done here, as we build SCCs. For our case it works even as is, with acceptable misbalance, but can be improved.


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

I am glad you think so. We will try to upstream it ASAP. The only question would be - same patch or an independent feature? Under common flag?

> If we can fix the correctness problem then I'd rather not have two implementations.

Ok. I can probably make it work with an enum flag for llvm::SplitModule or something.

Let me refactor the patch with EquivalenceClasses and single SplitModule function. Meanwhile Tobias will work on re-globalization patch. Stay tuned :)

Thanks.

Sergei

---
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation


> -----Original Message-----
> From: Peter Collingbourne [mailto:peter at pcc.me.uk]
> Sent: Tuesday, January 12, 2016 6:08 PM
> To: slarin at codeaurora.org; peter at pcc.me.uk
> Cc: mehdi.amini at apple.com; mehdi_amini at apple.com;
> tobias at codeaurora.org; llvm-commits at lists.llvm.org
> Subject: Re: [PATCH] D16124: Add to the split module utility an SCC based
> method which allows not to globalize any local variables.
> 
> 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
> 
>