[Openmp-commits] [PATCH] D108046: tsan: Add new annotation functions to promote all accesses to atomic

Fri Aug 13 12:15:30 PDT 2021

protze.joachim added a comment.

In D108046#2944211 <https://reviews.llvm.org/D108046#2944211>, @dvyukov wrote:

>> I must admit that this patch as it is causes ~10% runtime increase for one of my benchmarks that doesn't even use this annotation. If the patch is in general ok, we can optimize the performance.
>
> This is quite unfortunate. And it seems that the slow down is inherent to this approach. Or do you have some ideas of how to remove the overhead from the hot path?

I just realized, that I did not compare really compare to the same base but rather used a built from 8/3 for comparison. After I rebuilt my base version from the same main commit the performance is consistent.
This also means, that within the last 10 days we introduced 10% performance regression into main.

I'll run some bisecting to find the commit.

> What I wonder is: OpenMP should compile reductions (all? most?) to actual atomic operations, no? Or it should insert some kind of mutex lock/unlock around reduction code.
> Either way if we just expose the actual situation to tsan, it should work the way you want.
> This side-channel turning of non-atomic accesses into atomics looks strange. If the actual access are atomic, then we should just not lie to tsan that they are non-atomic in the place. And if the actual accesses are non-atomic, then we are masking real bugs.
> What am I missing?

The OpenMP runtime / compiler codegen use different strategies to implement the reduction. In some cases actually atomics are used. In other cases, the reduction is implemented as part of a barrier. Wearing my OpenMP application developer hat, I trust the OpenMP implementation. From this perspective casting memory accesses implementing the reduction to atomic makes sense to me.

Semantically I could also annotate Mutex semantics. But, from my understanding this introduces HappensBefore edges into the analysis, where I don't want to have them. They would hide even more data races than what I try to solve.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D108046/new/

https://reviews.llvm.org/D108046