[InstrProf] Use atomic profile counter updates for TSan

[vsk]

Summary

Thread sanitizer instrumentation fails to skip all loads and stores to
profile counters. This can happen if profile counter updates are merged:

%.sink = phi i64* ...
%pgocount5 = load i64, i64* %.sink
%27 = add i64 %pgocount5, 1
%28 = bitcast i64* %.sink to i8*
call void @__tsan_write8(i8* %28)
store i64 %27, i64* %.sink

To suppress TSan diagnostics about racy counter updates, make the
counter updates atomic when TSan is enabled. If there's general interest
in this mode it can be surfaced as a clang/swift driver option.

rdar://40477803

[jfb]

llvm/lib/Transforms/Instrumentation/InstrProfiling.cpp

608 ↗

(On Diff #161113)

Counters could just be relaxed, no need to use seq_cst.

[vsk]

llvm/lib/Transforms/Instrumentation/InstrProfiling.cpp

608 ↗

(On Diff #161113)

Thanks for calling this out.

[davidxl]

lgtm

cfe/trunk/lib/CodeGen/BackendUtil.cpp

if (Optional<GCOVOptions> Options = getGCOVOptions(CodeGenOpts)) {
     if (CodeGenOpts.getDebugInfo() == codegenoptions::NoDebugInfo)
       MPM.add(createStripSymbolsPass(true));
   }
 
   if (CodeGenOpts.hasProfileClangInstr()) {
     InstrProfOptions Options;
     Options.NoRedZone = CodeGenOpts.DisableRedZone;
     Options.InstrProfileOutput = CodeGenOpts.InstrProfileOutput;
+
+    // TODO: Surface the option to emit atomic profile counter increments at
+    // the driver level.
+    Options.Atomic = LangOpts.Sanitize.has(SanitizerKind::Thread);
+
     MPM.add(createInstrProfilingLegacyPass(Options));
   }
   if (CodeGenOpts.hasProfileIRInstr()) {
     PMBuilder.EnablePGOInstrGen = true;
     if (!CodeGenOpts.InstrProfileOutput.empty())
       PMBuilder.PGOInstrGen = CodeGenOpts.InstrProfileOutput;
     else
       PMBuilder.PGOInstrGen = DefaultProfileGenName;

cfe/trunk/test/CodeGen/tsan-instrprof-atomic.c

+// RUN: %clang_cc1 %s -emit-llvm -fprofile-instrument=clang -fsanitize=thread -o - | FileCheck %s
+
+// CHECK: define void @foo
+// CHECK-NOT: load {{.*}}foo
+// CHECK: ret void
+void foo() {}

llvm/trunk/include/llvm/Transforms/Instrumentation.h

 /// Options for the frontend instrumentation based profiling pass.
 struct InstrProfOptions {
   // Add the 'noredzone' attribute to added runtime library calls.
   bool NoRedZone = false;
 
   // Do counter register promotion
   bool DoCounterPromotion = false;
 
+  // Use atomic profile counter increments.
+  bool Atomic = false;
+
   // Name of the profile file to use as output
   std::string InstrProfileOutput;
 
   InstrProfOptions() = default;
 };
 
 /// Insert frontend instrumentation based profiling.
 ModulePass *createInstrProfilingLegacyPass(

llvm/trunk/lib/Transforms/Instrumentation/InstrProfiling.cpp

cl::opt<double> NumCountersPerValueSite(
     cl::desc("The average number of profile counters allocated "
              "per value profiling site."),
     // This is set to a very small value because in real programs, only
     // a very small percentage of value sites have non-zero targets, e.g, 1/30.
     // For those sites with non-zero profile, the average number of targets
     // is usually smaller than 2.
     cl::init(1.0));
 
+cl::opt<bool> AtomicCounterUpdateAll(
+    "instrprof-atomic-counter-update-all", cl::ZeroOrMore,
+    cl::desc("Make all profile counter updates atomic (for testing only)"),
+    cl::init(false));
+
 cl::opt<bool> AtomicCounterUpdatePromoted(
     "atomic-counter-update-promoted", cl::ZeroOrMore,
     cl::desc("Do counter update using atomic fetch add "
              " for promoted counters only"),
     cl::init(false));
 
 // If the option is not specified, the default behavior about whether
 // counter promotion is done depends on how instrumentaiton lowering

 }
 
 void InstrProfiling::lowerIncrement(InstrProfIncrementInst *Inc) {
   GlobalVariable *Counters = getOrCreateRegionCounters(Inc);
 
   IRBuilder<> Builder(Inc);
   uint64_t Index = Inc->getIndex()->getZExtValue();
   Value *Addr = Builder.CreateConstInBoundsGEP2_64(Counters, 0, Index);
+
+  if (Options.Atomic || AtomicCounterUpdateAll) {
+    Builder.CreateAtomicRMW(AtomicRMWInst::Add, Addr, Inc->getStep(),
+                            AtomicOrdering::Monotonic);
+  } else {
     Value *Load = Builder.CreateLoad(Addr, "pgocount");
     auto *Count = Builder.CreateAdd(Load, Inc->getStep());
     auto *Store = Builder.CreateStore(Count, Addr);
-  Inc->replaceAllUsesWith(Store);
     if (isCounterPromotionEnabled())
       PromotionCandidates.emplace_back(cast<Instruction>(Load), Store);
+  }
   Inc->eraseFromParent();
 }
 
 void InstrProfiling::lowerCoverageData(GlobalVariable *CoverageNamesVar) {
   ConstantArray *Names =
       cast<ConstantArray>(CoverageNamesVar->getInitializer());
   for (unsigned I = 0, E = Names->getNumOperands(); I < E; ++I) {
     Constant *NC = Names->getOperand(I);

llvm/trunk/test/Instrumentation/InstrProfiling/atomic-updates.ll

+; RUN: opt < %s -S -instrprof -instrprof-atomic-counter-update-all | FileCheck %s
+
+target triple = "x86_64-apple-macosx10.10.0"
+
+@__profn_foo = hidden constant [3 x i8] c"foo"
+
+; CHECK-LABEL: define void @foo
+; CHECK-NEXT: atomicrmw add i64* getelementptr inbounds ([1 x i64], [1 x i64]* @__profc_foo, i64 0, i64 0), i64 1 monotonic
+define void @foo() {
+  call void @llvm.instrprof.increment(i8* getelementptr inbounds ([3 x i8], [3 x i8]* @__profn_foo, i32 0, i32 0), i64 0, i32 1, i32 0)
+  ret void
+}
+
+declare void @llvm.instrprof.increment(i8*, i64, i32, i32)