Page MenuHomePhabricator

Add new files for OMPT
ClosedPublic

Authored by Hahnfeld on Apr 9 2015, 1:22 PM.

Details

Reviewers
Hahnfeld
Summary

No changes in the existing runtime and therefore no calls to the new functions.

Diff Detail

Event Timeline

Hahnfeld updated this revision to Diff 23482.Apr 9 2015, 1:22 PM
Hahnfeld retitled this revision from to Add new files for OMPT.
Hahnfeld updated this object.
Hahnfeld edited the test plan for this revision. (Show Details)
Hahnfeld added a subscriber: Unknown Object (MLST).
jlpeyton added inline comments.
runtime/CMakeLists.txt
611

You can remove this if(${OMPT_SUPPORT}) conditional. Because it is similar to a Makefile recipe, it is only called if the ompt.h file is part of the exported files.

851

This conditional is unnecessary for same reasons as above.

Hahnfeld added inline comments.Apr 10 2015, 6:29 AM
runtime/CMakeLists.txt
611

Ok, will update this with the next revision

I just noticed that there is a CMakeLists.txt in runtime/src/ as well. Is it deprecated?

omalyshe added inline comments.
runtime/src/ompt-internal.h
7

I suggest using "1.0".

runtime/src/ompt-specific.c
32

Can you consider using KMP_TEST_THEN_INC* macro instead (src/kmp_os.h) to make it working on Windows?

I just noticed that there is a CMakeLists.txt in runtime/src/ as well. Is it deprecated?

I'm not sure who this belongs to honestly... Can someone else pipe in here?

  • Johnny

I just noticed that there is a CMakeLists.txt in runtime/src/ as well. Is it deprecated?

I'm not sure who this belongs to honestly... Can someone else pipe in here?

  • Johnny

I can safely remove the file and the build with CMake is still working...
This is probably a relict of the first CMake build system that can still be used with runtime/CMakeLists.txt.old (btw: this gives me an error and doesn't work at all)

Back to OMPT:
Do you want me to post updated revisions? Till now we have only included your feedback as marked in the other comments...

runtime/src/ompt-internal.h
7

The Technical Report expects the version number as unsigned int...

page 19,

int ompt_initialize(ompt_function_lookup_t lookup, const char *runtime_version, unsigned int ompt_version);

runtime/src/ompt-specific.c
32

We have tested the change and everything works as expected (only looking at the macro, there shouldn't be any difference for Unix systems at all: It evaluates to __sync_fetch_and_add)

omalyshe added inline comments.Apr 16 2015, 2:28 PM
runtime/src/ompt-specific.c
32

Yes, for Unix systems KMP_TEST_THEN_INC* macro evaluates to sync_fetch_and_add but not for Windows. As I understand the OMPT code should work on Windows as well but with sync_fetch_and_add it couldn't be even compiled.

Hahnfeld added inline comments.Apr 16 2015, 2:32 PM
runtime/src/ompt-specific.c
32

Sorry, not clear from my side: We have changed it to KMP_TEST_THEN_INC64 and didn't see any problems.

Hahnfeld updated this revision to Diff 24292.Apr 23 2015, 12:44 PM

LGTM. Please, commit.

Hahnfeld accepted this revision.May 7 2015, 6:17 AM
Hahnfeld added a reviewer: Hahnfeld.

Committed with revision 236117

This revision is now accepted and ready to land.May 7 2015, 6:17 AM
Hahnfeld closed this revision.May 7 2015, 6:17 AM