These look fine to me.

LGTM

In D97079#2594807, @AndreyChurbanov wrote:

In 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.

@tlwilmar , you can just close this revision manually, but if you look at the git log for the llvm-project you'll see the "Differential Revision: <hyperlink>" line in many of the commits.

In D97019#2606795, @tlwilmar wrote:

Hi Johnny,
When I just commit via the instructions on our wiki, I just see this separate e-mail labeled "[Diffusion]" which looks like a separate review in Phabricator. When others push their changes, it appears attached to the original phabricator review, and seems to close it automagically. What am I missing there?

Thanks!

Ok, this actually restores using OLD_CMAKE_REQUIRED_FLAGS

Ok, I see what you're saying. I've copied the exact idiom used in the LLVM CMake where CMAKE_REQUIRED_FLAGS is temporarily stored in OLD_CMAKE_REQUIRED_FLAGS and then restored after the check.

LGTM

In D97413#2587137, @hans wrote:

Oh, and I just realized: just because my machine supports RTM doesn't necessarily mean we want to use it, since these builds will then be used on all kinds of machines. This might be a bigger problem with the runtime's build config though.

Since CMake's documentation states that the CMAKE_REQURIED_FLAGS variable is a string rather than a list, use simple set() to assign it.

Fixed so that cores is displayed in the warning message.

In D95170#2513762, @protze.joachim wrote:

In my quick test, env KMP_SETTINGS=1 OMP_PROC_BIND=spread ./a.out reports OMP_PLACES='cores'. I could not find the code path that sets places to cores in this case, but I think it would be more consistent to have consistent setting in the unset and the invalid case.

Is there a reason to choose threads?

Updated with documentation. Class is now kmp_safe_raii_file_t

LGTM now.

Sorry, I missed one thing.

In D91189#2407339, @mstorsjo wrote:

This broke compilation for me:

LGTM

one comment.

LGTM as well

LGTM as well.

ping

Fixed the GOMP dependency type assignment. Checked with the follow-up OMPT changes in D87680.

Change to XFAIL for clang and icc and add comment in test file.

Add #pragma omp task if(0) depend() version of taskwait depend test.

Use my_sleep() from omp_my_sleep.h

Test was poorly constructed. Second attempt.

Added enum for GOMP task flags.