[Openmp-commits] [PATCH] D14027: [OMPT] Windows Support for OMPT
John Mellor-Crummey via Openmp-commits
openmp-commits at lists.llvm.org
Fri Oct 23 14:13:52 PDT 2015
jmellorcrummey added a comment.
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
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.
More information about the Openmp-commits