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

[Xinliang David Li via llvm-commits]

On Tue, Mar 15, 2016 at 4:28 PM, Anna Zaks via llvm-commits <
llvm-commits at lists.llvm.org> wrote:

> zaks.anna added a comment.
>
> > We are probably talking about different things here.  My main concern is
> the additional constraints
>
> >  imposed on the optimizer that can penalize optimizations (no-op or not
> on a target).
>
>
> I see. Are there specific benchmarks that I could use for testing this
> impact?
>


SPEC or instrumented clang itself can be reasonable.  However the concern
is  also about future optimizations -- more aggressive global optimizations
on counter accesses (See Vedant's recent RFC about reducing instrumentation
overhead).


>
> > Since it is known certain combinations of instrumentions do not work
> together, why not documenting the
>
> >  behavior and let user filter out the false warnings?
>
>
> These are not corner case combinations. We encourage users to run their
> tests with code coverage. At the same time, we advise to use sanitizers for
> bug finding. It is very likely that people will want to have both in their
> CI or when running tests.
>

so the use case include configs like:

 - coverage + tsan
 - coverage + asan
 - coverage + ...san ?


>
> > Back to your proposal to use Monotonic load. It does not fix the actual
> race (to fix the race, atomic
>
> >  fetch_add instruction is needed) nor can it silence tsan.
>
>
> It does silence TSan, which is what I mainly care about. TSan performs
> it's instrumentation on LLVM IR level, where it sees the atomic load/store.
>

In the small case I tried, tsan  replaces the monotonic load with
__tsan_atomic_load32(..) call -- i still saw the runtime warning.


>
> > Having said that, I think it is reasonable to introduce an option to
> enable atomic profile counter update.
>
> >  Tsan can then be safely combined with that.
>
>
> Are you saying that you are OK with having this option but it should not
> be turned on by default unless we can show that the overhead of using
> Monotonic is minimal?
>
>

More than that. My suggestion is do it more aggressively. Introduce an
option to enable lowering the counter increment into

__atomic_fetch_add(&counter, 1, __ATOMIC_RELAXED)

or

.. =  atomicrmw add ... monotonic

at IR level.

This should be off by default.

David





>
> http://reviews.llvm.org/D18164
>
>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20160315/ad1e3afa/attachment.html>