This is an archive of the discontinued LLVM Phabricator instance.

Add new files for OMPT
ClosedPublic

Authored by Hahnfeld on Apr 9 2015, 6:22 AM.

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, 6:22 AM
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
612

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.

849

This conditional is unnecessary for same reasons as above.

Hahnfeld added inline comments.Apr 9 2015, 11:29 PM
runtime/CMakeLists.txt
612

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
8

I suggest using "1.0".

runtime/src/ompt-specific.c
33

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
8

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
33

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, 7:28 AM
runtime/src/ompt-specific.c
33

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, 7:32 AM
runtime/src/ompt-specific.c
33

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, 5:44 AM

LGTM. Please, commit.

Hahnfeld accepted this revision.May 6 2015, 11:17 PM
Hahnfeld added a reviewer: Hahnfeld.

Committed with revision 236117

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