Page MenuHomePhabricator

[OMPT] Rename ompt_mutex_impl_t to kmp_mutex_impl
ClosedPublic

Authored by sconvent on Dec 13 2017, 3:44 AM.

Details

Summary

The defintion is not part of the spec and thus should not have the prefix "ompt_" but rather a prefix that indicates that this is implementation specific.

Diff Detail

Repository
rL LLVM

Event Timeline

sconvent created this revision.Dec 13 2017, 3:44 AM
sconvent retitled this revision from Move definition of ompt_mutex_impl_t to ompt-internal.h to [OMPT] Move definition of ompt_mutex_impl_t to ompt-internal.h.Dec 13 2017, 3:49 AM

Makes sense in some way. However if I read the spec correctly the tool gets an unsigned int. So maybe we should keep the enum in the header but prefix it with kmp instead of ompt to make clear that this is not portable? This would allow tools to conditionally compare the passed value

sconvent updated this revision to Diff 127842.Dec 21 2017, 1:17 AM
sconvent retitled this revision from [OMPT] Move definition of ompt_mutex_impl_t to ompt-internal.h to [OMPT] Rename ompt_mutex_impl_t to kmp_mutex_impl.
sconvent edited the summary of this revision. (Show Details)

Implemented suggestion

Hahnfeld requested changes to this revision.Dec 21 2017, 2:43 AM
Hahnfeld added inline comments.
runtime/src/include/50/ompt.h.var
87 ↗(On Diff #127842)

This is actually defined in the spec, so we need to keep it prefixed with ompt

This revision now requires changes to proceed.Dec 21 2017, 2:43 AM
sconvent updated this revision to Diff 129222.Jan 10 2018, 2:20 AM

Implemented suggestion

sconvent added inline comments.Jan 10 2018, 2:22 AM
runtime/src/include/50/ompt.h.var
87 ↗(On Diff #127842)

You're right. The spec specifies this value as a macro however (not an enum). I think we should wait for feedback on Github Issue 112

protze.joachim accepted this revision.Jan 17 2018, 1:02 AM

We discussed to relax the wording in the OpenMP spec, so that any implementation that provides ompt_mutex_impl_unknown with value 0 is complying. This will include #define, enum or "static const" values.

Hahnfeld accepted this revision.Jan 17 2018, 1:14 AM

LGTM then

This revision is now accepted and ready to land.Jan 17 2018, 1:14 AM
This revision was automatically updated to reflect the committed changes.