This is an archive of the discontinued LLVM Phabricator instance.

Overhaul OMPT initialization interface
ClosedPublic

Authored by jmellorcrummey on Sep 19 2015, 4:16 PM.

Details

Summary

The OMPT specification has changed. This revision brings the LLVM OpenMP implementation up to date.

Technical overview of changes:

Previously, a public weak symbol ompt_initialize was called after the OpenMP runtime is initialized. The new interface calls a global weak symbol ompt_tool prior to initialization. If a tool is present, ompt_tool returns a pointer to a function that matches the signature for ompt_initialize. After OpenMP is initialized the function pointer is called to initialize a tool.

Knowing that OMPT will be enabled before initialization allows OMPT support to be initialized as part of initialization instead of back patching initialization of OMPT support after the fact.

Post OpenMP initialization support has been generalized moves from ompt-specific.c into ompt-general.c, since the OMPT initialization logic is no longer implementation specific.

Diff Detail

Repository
rL LLVM

Event Timeline

jmellorcrummey retitled this revision from to Overhaul OMPT initialization interface.
jmellorcrummey updated this object.
jmellorcrummey set the repository for this revision to rL LLVM.
Hahnfeld accepted this revision.Sep 21 2015, 12:41 AM
Hahnfeld added a reviewer: Hahnfeld.
Hahnfeld added a subscriber: Hahnfeld.

LGTM

runtime/src/ompt-general.c
135 ↗(On Diff #35185)

Wrong indention (can be fixed on commit)

This revision is now accepted and ready to land.Sep 21 2015, 12:41 AM
omalyshe added inline comments.
runtime/src/ompt-general.c
115 ↗(On Diff #35185)

typo "iff" ->"if"

jlpeyton requested changes to this revision.Sep 21 2015, 8:36 AM
jlpeyton edited edge metadata.

See inline comments.

runtime/src/kmp_runtime.c
6379–6389 ↗(On Diff #35185)

Small nit, can you move this pre_init() call after the 5 KMP_DEBUG_ASSERT()'s?

6631–6633 ↗(On Diff #35185)

Small nit, can you move these three lines before the KMP_MB() above?

6768–6771 ↗(On Diff #35185)

This is unnecessary since middle_init() -> do_middle_init() -> serial_init() -> do_serial_init(). Plus, the ompt_[pre|post]_init()'s in the do_serial_init() are guarded by the __kmp_initz_lock. The guarentee with all this init() code is that do_serial_init() will happen first and be executed safely by a single thread.

6792–6795 ↗(On Diff #35185)

Unnecessary because parallel_init() -> do_middle_init() and so on.

6859 ↗(On Diff #35185)

Unnecessary as well since this parallel_init() -> ... -> do_serial_init().

runtime/src/ompt-general.c
55–59 ↗(On Diff #35185)

In addition to the indention problem Jonas pointed out, have these be four spaces as well.

115 ↗(On Diff #35185)

Olga, this is short for "if and only if". At least I believe that is what is meant here.

This revision now requires changes to proceed.Sep 21 2015, 8:36 AM

Sorry, I missed one.

runtime/src/kmp_runtime.c
6783 ↗(On Diff #35185)

I forgot this one, this is unnecessary as well.

jmellorcrummey edited edge metadata.
jmellorcrummey removed rL LLVM as the repository for this revision.

Addressed comments:

  • moved the call to ompt_pre_init to after the calls to KMP_DEBUG_ASSERT in __kmp_do_serial_initialize
  • moved the call to ompt_post_init before the call to KMP_MB in __kmp_do_serial_initialize
  • removed redundant calls to ompt_pre_init/post_init in kmp_parallel_initialize and kmp_middle_initialize
  • fixed indentation issues caused by tabs
  • fixed typos in two comments (a missing "is" in the comment with iff and a misspelled synchronize in a pre-existing comment).
jlpeyton accepted this revision.Sep 21 2015, 10:38 AM
jlpeyton edited edge metadata.

Thanks! LGTM. Will commit.

This revision is now accepted and ready to land.Sep 21 2015, 10:38 AM
This revision was automatically updated to reflect the committed changes.