Page MenuHomePhabricator

[OMPT] Windows Support for OMPT
ClosedPublic

Authored by jlpeyton on Oct 23 2015, 12:38 PM.

Details

Summary

The problem is that the ompt_tool() function (which must be implemented by a performance tool) should be defined in the RTL as well to cover the case when the tool is not present in the address space of the process. This functionality is accomplished with weak symbols in Unices. Unfortunately, Windows does not support weak symbols.

The solution in these changes is to grab the list of all modules loaded by the process and then search for symbol "ompt_tool()" within them. The function ompt_tool_windows() performs the search of the ompt_tool symbol. If ompt_tool is found, then its return value is used to initialize the tool. If ompt_tool is not found, then ompt_tool_windows() returns NULL and OMPT is thus, disabled.

While doing these changes, the OMPT_SUPPORT detection in CMake was changed to test for the required featuers for OMPT_SUPPORT, namely: builtin_frame_address() existence, weak attribute existence and psapi.dll existence. For LIBOMP_HAVE_OMPT_SUPPORT to be true, it must be that the builtin_frame_address() intrinsic exists
AND one of: either weak attributes exist or psapi.dll exists.

Also, since Process Status API is used I had to add new dependency -- psapi.dll to the library dependency micro test.

Diff Detail

Repository
rL LLVM

Event Timeline

jlpeyton updated this revision to Diff 38251.Oct 23 2015, 12:38 PM
jlpeyton retitled this revision from to [OMPT] Windows Support for OMPT.
jlpeyton updated this object.
jlpeyton added reviewers: omalyshe, jmellorcrummey.
jlpeyton set the repository for this revision to rL LLVM.
jlpeyton added a subscriber: openmp-commits.

While the changes to the build system seem fine to me, the changes to ompt-general.c are awkward. I don't like the "#if KMP_HAVE_PSAPI" special casing inside ompt_pre_init. Also, there is no need for a function ompt_aux_tool. I recommend the following instead:

In ompt_pre_init, I recommend removing all ifdefs around the line

ompt_initialize_fn = ompt_tool();

Instead, at the top of the file I recommend that under "#elif KMP_HAVE_PSAPI" you eliminate the code for ompt_aux_tool and instead have

#define ompt_tool ompt_windows_ompt_tool

ompt_initialize_t ompt_tool_windows()
{

...

}

The implementation of ompt_tool_windows should be based on the implementation for what you call ompt_symbol_lookup with two changes. First, the name of the symbol to look up "ompt_tool" should be "baked in". OMPT relies only on one public symbol by design; there will never be a need to look up another function in a tool, so the functionality provided by ompt_symbol_lookup is overkill. Second, in all cases where __ompt_symbol_lookup returns &ompt_aux_tool, ompt_tool_windows should just return NULL. Third, rather than breaking and returning the address of the function fn returned by

fn = GetProcAddress(modules[i], "ompt_tool")

ompt_tool_windows should just return the value returned by invoking fn.

These changes eliminate the need for the useless function ompt_aux_tool, eliminate the excess functionality provided by __ompt_symbol_lookup, and eliminate special case handling for Windows inside ompt_pre_init.

Two more things:

First, I made some name changes in the midst of writing my prior comment. I didn't change one occurrence. The corrected #define for the prior comment should be

#define ompt_tool ompt_tool_windows

Second, I note that the implementation

#if KMP_HAVE_WEAK_ATTRIBUTE

#elif KMP_HAVE_PSAPI

#endif

will fail with an undefined symbol for ompt_tool if neither KMP_HAVE_WEAK_ATTRIBUTE or KMP_HAVE_PSAPI are defined. Should there be an #else case with a #error that says that either weak symbols or libpsapi are required.

One final comment:

I designed ompt-general.c as a source file that could be used by anyone any runtime system to implement OMPT. Before this revision, there was no dependence of this file on anything to do with Intel runtime. This change introduces several KMP defines to this runtime. I would prefer that the names of defines used in this file have the OMPT prefix, e.g. OMPT_WEAK_ATTRIBUTE, OMPT_PSAPI, and OMPT_DEBUG. These defines could be set in ompt-specific.h (which is intended provide details for tailoring OMPT for any particular runtime) as

#define OMPT_WEAK_ATTRIBUTE KMP_WEAK_ATTRIBUTE
#define OMPT_PSAPI KMP_PSAPI
#define OMPT_DEBUG KMP_TRACE

I was wrong. I had one more thing to say :-).

Changing strcasecmp for argument matching to __kmp_str_match is another change that binds the ompt-general.c implementation to the Intel runtime.

I would prefer adding a layer of abstraction to this if you want to use __kmp_str_match. I recommend that ompt-specific.h have the following line added

#define OMPT_STR_MATCH(haystack, needle) __kmp_str_match(haystack, 0, needle)

and then in ompt-general.c, include the following line

#ifndef OMPT_STR_MATCH
#define OMPT_STR_MATCH(haystack, needle) (!strcasecmp(haystack, needle))
#endif

jlpeyton updated this revision to Diff 38751.Oct 29 2015, 11:14 AM
jlpeyton updated this object.
jlpeyton edited edge metadata.

Addressed all of John Mellor-Crummey's suggestions. (Did them all)

I have one more minor comment

Lines 104 and 105 of ompt-general.c should be reversed.

_OMP_EXTERN
#if OMPT_HAVE_WEAK_ATTRIBUTE

should be

#if OMPT_HAVE_WEAK_ATTRIBUTE
_OMP_EXTERN

because the _OMP_EXTERN does not apply to the #elseif case.

Besides that, everything now looks good to me.

This revision was automatically updated to reflect the committed changes.