[PATCH] D18164: [tsan] Do not instrument reads/writes to instruction profile counters.
Xinliang David Li via llvm-commits
llvm-commits at lists.llvm.org
Tue Mar 15 16:48:07 PDT 2016
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
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
> > 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)
.. = atomicrmw add ... monotonic
at IR level.
This should be off by default.
> llvm-commits mailing list
> llvm-commits at lists.llvm.org
-------------- next part --------------
An HTML attachment was scrubbed...
More information about the llvm-commits