[PATCH] D11579: [InstrProfiling] Fix data race on profile counters by using AtomicRMW.

[Bob Wilson]

> On Jul 29, 2015, at 4:56 PM, Alexey Samsonov <vonosmas at gmail.com> wrote:
> 
> Do relaxed atomics actually introduce that much of slowdown?

I would definitely want to see some data showing that they do not slow things down before we decide to do this unconditionally. We’ve discussed this issue several times in the past. My recollection is those discussions ended with an acknowledgement there is a tradeoff between speed and accuracy and that we don’t all agree on where we want to be on that spectrum. Adding options to let people choose would be one solution. Good data, on a variety of platforms, showing that it doesn’t make much difference would be another way to resolve it.

> 
> You're intentionally introducing a data race, this doesn't look good to me at all. However, I'm not confident about
> what's allowed in LLVM IR - it's not C++ where any source-level data race is UB, but not an x86 assembly either.
> 
> On Wed, Jul 29, 2015 at 4:43 PM, Justin Bogner <mail at justinbogner.com <mailto:mail at justinbogner.com>> wrote:
> Alexey Samsonov <vonosmas at gmail.com <mailto:vonosmas at gmail.com>> writes:
> > samsonov created this revision.
> > samsonov added reviewers: dnovillo, bogner.
> > samsonov added a subscriber: llvm-commits.
> >
> > Since we introduced counters for functions in COMDAT sections (e.g.
> > inline functions from STL headers), these headers can easily be
> > incremented concurrently by multiple threads. Replace load-add-store
> > with a single "atomicrmw add" with monotonic memory ordering.
> 
> This significantly changes the performance characteristics of this code,
> pessimizing single-threaded users and potentially making the
> multithreaded performance issues even worse.
> 
> It's fine to add an option to lower these to atomics, since this does
> guarantee accuracy, but I think we need a switch to choose which kind of
> lowering we're interested in in that case.
> 
> > http://reviews.llvm.org/D11579 <https://urldefense.proofpoint.com/v2/url?u=http-3A__reviews.llvm.org_D11579&d=AwMFaQ&c=8hUWFZcy2Z-Za5rBPlktOQ&r=mQ4LZ2PUj9hpadE3cDHZnIdEwhEBrbAstXeMaFoB9tg&m=lDx1rX-3B32oZTA_vCe21kRN0Y14ujW2ePnnU3JiUX4&s=tiiUXsu0al8aXyrPKSc28tcPx4YG5wxgSglC63-ASTQ&e=>
> >
> > Files:
> >   lib/Transforms/Instrumentation/InstrProfiling.cpp
> >
> > Index: lib/Transforms/Instrumentation/InstrProfiling.cpp
> > ===================================================================
> > --- lib/Transforms/Instrumentation/InstrProfiling.cpp
> > +++ lib/Transforms/Instrumentation/InstrProfiling.cpp
> > @@ -147,9 +147,9 @@
> >    IRBuilder<> Builder(Inc->getParent(), *Inc);
> >    uint64_t Index = Inc->getIndex()->getZExtValue();
> >    Value *Addr = Builder.CreateConstInBoundsGEP2_64(Counters, 0, Index);
> > -  Value *Count = Builder.CreateLoad(Addr, "pgocount");
> > -  Count = Builder.CreateAdd(Count, Builder.getInt64(1));
> > -  Inc->replaceAllUsesWith(Builder.CreateStore(Count, Addr));
> > +  Builder.CreateAtomicRMW(AtomicRMWInst::Add, Addr, Builder.getInt64(1),
> > +                          llvm::Monotonic);
> > +  assert(Inc->use_empty() && "InstrProfIncrementInst has uses!");
> >    Inc->eraseFromParent();
> >  }
> 
> 
> 
> 
> -- 
> Alexey Samsonov
> vonosmas at gmail.com <mailto:vonosmas at gmail.com>_______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu <mailto:llvm-commits at cs.uiuc.edu>
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits <http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150729/b3fed82b/attachment.html>