[llvm] r245752 - Range-for-ify some things in GlobalMerge

[David Blaikie via llvm-commits]

On Tue, Aug 25, 2015 at 3:25 AM, Charlie Turner <charlesturner7c5 at gmail.com>
wrote:

> Hi David, I think there's a mistake here:
>
> You've replaced
>
> -  if (EnableGlobalMergeOnConst)
> -    for (DenseMap<unsigned, SmallVector<GlobalVariable*, 16> >::iterator
> -         I = ConstGlobals.begin(), E = ConstGlobals.end(); I != E; ++I)
> -      if (I->second.size() > 1)
> -        Changed |= doMerge(I->second, M, true, I->first);
>
> with
>
> +  for (auto &P : ConstGlobals)
> +    if (P.second.size() > 1)
> +      Changed |= doMerge(P.second, M, true, P.first);
>
> Looks like you missed the if (EnableGlobalMergeOnConst) guard. I'm
> seeing a 3.69% regression in spec2000's 253.perlbmk with that guard
> removed.
>
> Could you take a look please?
>

Yep, looks like exactly that. Added the conditional back in r245954.

Thanks!
- Dave


>
> Thanks for your time,
> Charlie.
>
> On 21 August 2015 at 23:19, David Blaikie via llvm-commits
> <llvm-commits at lists.llvm.org> wrote:
> > Author: dblaikie
> > Date: Fri Aug 21 17:19:06 2015
> > New Revision: 245752
> >
> > URL: http://llvm.org/viewvc/llvm-project?rev=245752&view=rev
> > Log:
> > Range-for-ify some things in GlobalMerge
> >
> > Modified:
> >     llvm/trunk/lib/CodeGen/GlobalMerge.cpp
> >
> > Modified: llvm/trunk/lib/CodeGen/GlobalMerge.cpp
> > URL:
> http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/CodeGen/GlobalMerge.cpp?rev=245752&r1=245751&r2=245752&view=diff
> >
> ==============================================================================
> > --- llvm/trunk/lib/CodeGen/GlobalMerge.cpp (original)
> > +++ llvm/trunk/lib/CodeGen/GlobalMerge.cpp Fri Aug 21 17:19:06 2015
> > @@ -135,7 +135,7 @@ namespace {
> >                   Module &M, bool isConst, unsigned AddrSpace) const;
> >      /// \brief Merge everything in \p Globals for which the
> corresponding bit
> >      /// in \p GlobalSet is set.
> > -    bool doMerge(SmallVectorImpl<GlobalVariable *> &Globals,
> > +    bool doMerge(const SmallVectorImpl<GlobalVariable *> &Globals,
> >                   const BitVector &GlobalSet, Module &M, bool isConst,
> >                   unsigned AddrSpace) const;
> >
> > @@ -406,15 +406,14 @@ bool GlobalMerge::doMerge(SmallVectorImp
> >    return Changed;
> >  }
> >
> > -bool GlobalMerge::doMerge(SmallVectorImpl<GlobalVariable *> &Globals,
> > +bool GlobalMerge::doMerge(const SmallVectorImpl<GlobalVariable *>
> &Globals,
> >                            const BitVector &GlobalSet, Module &M, bool
> isConst,
> >                            unsigned AddrSpace) const {
> > +  assert(Globals.size() > 1);
> >
> >    Type *Int32Ty = Type::getInt32Ty(M.getContext());
> >    auto &DL = M.getDataLayout();
> >
> > -  assert(Globals.size() > 1);
> > -
> >    DEBUG(dbgs() << " Trying to merge set, starts with #"
> >                 << GlobalSet.find_first() << "\n");
> >
> > @@ -562,21 +561,17 @@ bool GlobalMerge::doInitialization(Modul
> >      }
> >    }
> >
> > -  for (DenseMap<unsigned, SmallVector<GlobalVariable*, 16> >::iterator
> > -       I = Globals.begin(), E = Globals.end(); I != E; ++I)
> > -    if (I->second.size() > 1)
> > -      Changed |= doMerge(I->second, M, false, I->first);
> > -
> > -  for (DenseMap<unsigned, SmallVector<GlobalVariable*, 16> >::iterator
> > -       I = BSSGlobals.begin(), E = BSSGlobals.end(); I != E; ++I)
> > -    if (I->second.size() > 1)
> > -      Changed |= doMerge(I->second, M, false, I->first);
> > -
> > -  if (EnableGlobalMergeOnConst)
> > -    for (DenseMap<unsigned, SmallVector<GlobalVariable*, 16> >::iterator
> > -         I = ConstGlobals.begin(), E = ConstGlobals.end(); I != E; ++I)
> > -      if (I->second.size() > 1)
> > -        Changed |= doMerge(I->second, M, true, I->first);
> > +  for (auto &P : Globals)
> > +    if (P.second.size() > 1)
> > +      Changed |= doMerge(P.second, M, false, P.first);
> > +
> > +  for (auto &P : BSSGlobals)
> > +    if (P.second.size() > 1)
> > +      Changed |= doMerge(P.second, M, false, P.first);
> > +
> > +  for (auto &P : ConstGlobals)
> > +    if (P.second.size() > 1)
> > +      Changed |= doMerge(P.second, M, true, P.first);
> >
> >    return Changed;
> >  }
> >
> >
> > _______________________________________________
> > llvm-commits mailing list
> > llvm-commits at lists.llvm.org
> > http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150825/af5fd0c3/attachment.html>