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

[David Li via llvm-commits]

davidxl added a subscriber: davidxl.

================
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);
+
----------------
Document the right behavior here -- Return true when ..., otherwise return false.

================
Comment at: lib/ProfileData/InstrProf.cpp:252
@@ -250,1 +251,3 @@
 
+bool isAddressOfProfileCounter(Value *Addr) {
+  // If this is a GEPCE, just analyze its pointer operand.
----------------
If I read the code correctly,  the logic seems to be the opposite of what it is suppose to mean --  It should return true when the variable is in profile counter section, not the 'false'

================
Comment at: lib/ProfileData/InstrProf.cpp:257
@@ +256,3 @@
+      Addr = CE->getOperand(0);
+
+  // Check if global the address is pointing to is in the
----------------
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?


http://reviews.llvm.org/D18164