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

[David Li via llvm-commits]

davidxl added inline comments.

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


http://reviews.llvm.org/D18164