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

[Alexey Samsonov]

Do relaxed atomics actually introduce that much of slowdown?

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>
wrote:

> Alexey Samsonov <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
> >
> > 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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150729/7b92eaa3/attachment.html>