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

[Justin Bogner]

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();
>  }