[PATCH] D18164: [tsan] Do not instrument reads/writes to instruction profile counters.

[Anna Zaks via llvm-commits]

zaks.anna added inline comments.

================
Comment at: include/llvm/ProfileData/InstrProf.h:239
@@ +238,3 @@
+/// Checks if the provided Value is an address of the profiler counter.
+bool isAddressOfProfileCounter(Value *Addr);
+
----------------
davidxl wrote:
> Document the right behavior here -- Return true when ..., otherwise return false.
Thanks for catching this!

================
Comment at: lib/ProfileData/InstrProf.cpp:257
@@ +256,3 @@
+      Addr = CE->getOperand(0);
+
+  // Check if global the address is pointing to is in the
----------------
davidxl wrote:
> vsk wrote:
> > I just realized that this isn't exhaustive. We can end up with loads/stores with non-GEP pointer operands. Consider the following example:
> > 
> > ```
> > % cc -O3 -fprofile-instr-generate -S -emit-llvm -x c - -o -
> > void foo() {}
> > 
> > int main() {
> >   for (int I = 0; I < 10; ++I) foo();
> > }
> > ```
> > 
> > This produces:
> > 
> > ```
> >   %1 = load <2 x i64>, <2 x i64>* bitcast ([2 x i64]* @__profc_main to <2 x i64>*), align 8
> >   %.promoted3 = load i64, i64* getelementptr inbounds ([1 x i64], [1 x i64]* @__profc_foo, i64 0, i64 0), align 8
> >   %2 = add i64 %.promoted3, 10
> >   %3 = add <2 x i64> %1, <i64 1, i64 10>
> >   store <2 x i64> %3, <2 x i64>* bitcast ([2 x i64]* @__profc_main to <2 x i64>*), align 8
> >   store i64 %2, i64* getelementptr inbounds ([1 x i64], [1 x i64]* @__profc_foo, i64 0, i64 0), align 8
> > ```
> > 
> > So I think we should add a check like this:
> > 
> > ```
> > if (auto *BCI = dyn_cast<BitCastInst>(Addr))
> >   Addr = BCI->getOperand(0);
> > ```
> I suppose there is common utility helper functions to do the stripping -- e.g. getBaseAddr or something like that?
Transforms/Utils would be a good place for this but I cannot find anything like it there. 

A function that strips everything up to the base pointer would be more involved, we'd need to loop/recurse until we hit something that's not a GEP constant, GEP instruction, BitCast and more?

So far, the intention here is to just pattern match what is known to be added by the profiler pass. Hopefully, this would be sufficiently robust.

================
Comment at: lib/ProfileData/InstrProf.cpp:265
@@ +264,3 @@
+      if (getInstrProfCountersSectionName(isMachO).equals(GV->getSection()))
+        return false;
+    }
----------------
vsk wrote:
> This is fine, but we can avoid constructing the Triple object if you're ok with doing `StringRef(GV->getSection()).endswith(getInstrProfCountersSectionName(/*AddSegment=*/false)`.
It's probably cleaner/more readable with the Triple. Are you worried about speed?


http://reviews.llvm.org/D18164