Page MenuHomePhabricator

[OpenMP] Implementation of OMPT reduction callbacks
ClosedPublic

Authored by protze.joachim on Nov 18 2019, 6:49 AM.

Details

Summary

Including two tests

These callbacks were added late to the 5.0 specification, an implementation is missing.

Diff Detail

Event Timeline

protze.joachim created this revision.Nov 18 2019, 6:49 AM
Herald added a project: Restricted Project. · View Herald Transcript
Herald added a subscriber: guansong. · View Herald Transcript

Apply clang-format

jdoerfert added inline comments.Nov 26 2019, 9:04 PM
openmp/runtime/src/kmp_barrier.cpp
138

These seem to be basically the same block(s) of code multiple times. Can we outline them into a helper (including the guards)?

openmp/runtime/test/ompt/callback.h
439–442

Can we not have default clauses so we get appropriate warnings?

Implement requested changes.

@jdoerfert is this as you intended the change?

Implement requested changes.

@jdoerfert is this as you intended the change?

Yes. I was expecting functions but macros seems to be the solution used in this code so far.
Do you think it is worth it to do it for openmp/runtime/src/kmp_csupport.cpp as well?

openmp/runtime/test/ompt/callback.h
417–420

default again.

Macro outsourced to ompt_specific, which is included by both kmp source files.
Tested for LIBOMP_OMPT_SUPPORT on/off builds.

Yes. I was expecting functions but macros seems to be the solution used in this code so far.

Macros are used, because variables are in the callers scope and re-used by the end macro.

Probably a question of style: should there be a do{}while(0) to catch the ;?
We need the ; after the macro invocation to please clang-format

jdoerfert accepted this revision.Dec 10 2019, 9:22 AM

Thx, looks much cleaner (IMHO) and good to me now.

Yes. I was expecting functions but macros seems to be the solution used in this code so far.

Macros are used, because variables are in the callers scope and re-used by the end macro.

I would have tried references/pointers but I guess macros are the state-of-the-art solution here.

Probably a question of style: should there be a do{}while(0) to catch the ;?
We need the ; after the macro invocation to please clang-format

Sure.

This revision is now accepted and ready to land.Dec 10 2019, 9:22 AM

Pushed as 3356e268.

Not sure why this DR was not closed.