[Openmp-commits] [PATCH] D97079: [OpenMP] libomp: eliminate pause from atomic CAS loops

[Jonathan Peyton via Phabricator via Openmp-commits]

jlpeyton added a comment.

In D97079#2594807 <https://reviews.llvm.org/D97079#2594807>, @AndreyChurbanov wrote:

> In D97079#2575663 <https://reviews.llvm.org/D97079#2575663>, @JonChesterfield wrote:
>
>> I think this is architecture specific.
>>
>> If a CAS failed spuriously, then immediately retry is good. If it failed because another core wrote to the cache line, then we have established that said cache line is somewhat contended, in which case pause() may let the other threads progress faster.
>
> If one thread succeeded then ALL other threads competing on the cache line would fail.  So they will not progress faster executing pause instruction.  Only gain can be for succeeding thread, that means it executes nothing but atomic in a loop, and that is not a real code example...
>
> Actually the atomic functions are never called by clang, so it is kind of cleanup change in any case.
>
> Experiments calling an internal atomic function directly in a loop show either no difference on some processors (where pause instruction is fast enough), or significant speedup (where pause instruction is slow).
>
> One clear gain of this patch is reduced size of the runtime - I see 4096 bytes smaller size of the binary if the patch applied.
>
> If you still think the patch is not good, I am fine to abandon it.  But to me it is pretty harmless, and as I mentioned, it reduces the size of the runtime.

I think we should keep this patch. These atomic functions are just trying to mimic what a hardware instruction would accomplish. In line with Andrey's reasoning, if a CAS fails here then there is nothing to wait for since the other thread that caused the fail is done with the atomic. i.e., the critical section is the atomic itself. Hence, the "spin wait" is already over and another retry should take place immediately.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97079