This is an archive of the discontinued LLVM Phabricator instance.

[OpenMP] Introduce an environment variable to disable atomic map clauses
ClosedPublic

Authored by jdoerfert on Jan 18 2022, 5:02 PM.

Details

Summary

Atomic handling of map clauses was introduced to comply with the OpenMP
standard (see D104418). However, many apps won't need this feature which
can be costly in certain situations. To allow for applications to
opt-out we now introduce the LIBOMPTARGET_MAP_FORCE_ATOMIC environment
flag that voids the atomicity guarantee of the standard for map clauses
again, shifting the burden to the user.

This patch also de-duplicates the code that introduces the events used
to enforce atomicity as a cleanup.

Diff Detail

Event Timeline

jdoerfert created this revision.Jan 18 2022, 5:02 PM
jdoerfert requested review of this revision.Jan 18 2022, 5:02 PM
Herald added a project: Restricted Project. · View Herald TranscriptJan 18 2022, 5:02 PM
Herald added a subscriber: sstefan1. · View Herald Transcript
tianshilei1992 added inline comments.Jan 18 2022, 5:25 PM
openmp/libomptarget/src/device.cpp
22

This is arguably not a good practice to handle locks. We have Entry->lock() in one place and then nothing. For sure the function name addEventIfNecessaryAndUnlock contains "unlock", but it's still too vague because locks are usually used by pair or guarded by scope. Although I agree the code looks more neat, I don't think it's worth to do it in exchange of ambiguity.

jdoerfert updated this revision to Diff 401063.Jan 18 2022, 5:37 PM

Address comments, use lock guard

jdoerfert marked an inline comment as done.Jan 18 2022, 5:37 PM
jdoerfert added inline comments.
openmp/libomptarget/src/device.cpp
22

done

tianshilei1992 accepted this revision.Jan 18 2022, 5:43 PM

LGTM

openmp/docs/design/Runtimes.rst
1015

Probably better to mention no matter implicitly or explicitly.

openmp/libomptarget/include/device.h
215

That's a beautiful solution!

This revision is now accepted and ready to land.Jan 18 2022, 5:43 PM
jdoerfert marked an inline comment as done.Jan 18 2022, 5:52 PM
jdoerfert added inline comments.
openmp/docs/design/Runtimes.rst
1015

I don't get that. What to mention and how?

ye-luo requested changes to this revision.Jan 18 2022, 6:39 PM
ye-luo added inline comments.
openmp/docs/design/Runtimes.rst
686

OpenMP spec use true/false, TRUE/FALSE. So better to keep the same convention. Values other than these four values should error out.

openmp/libomptarget/src/device.cpp
26

unlock before return.

This revision now requires changes to proceed.Jan 18 2022, 6:39 PM
ye-luo accepted this revision.Jan 18 2022, 7:33 PM

Please update the allowed string values and also resolve conflict before landing.

This revision is now accepted and ready to land.Jan 18 2022, 7:33 PM
tianshilei1992 added inline comments.Jan 19 2022, 8:07 AM
openmp/docs/design/Runtimes.rst
686

libomp supports all of them. Better to align with them.

1015

"As a consequence, users have to guarantee themselves that no two data mappings of same memory, explicitly through map clause or implicitly applied by compiler, will concurrently happen."

jdoerfert updated this revision to Diff 401245.Jan 19 2022, 8:07 AM

Address comments

jdoerfert added inline comments.Jan 19 2022, 8:10 AM
openmp/libomptarget/src/device.cpp
26

guards fix this nicely :)

This revision was landed with ongoing or failed builds.Jan 19 2022, 8:15 PM
This revision was automatically updated to reflect the committed changes.