[Openmp-commits] [PATCH] D14027: [OMPT] Windows Support for OMPT

[John Mellor-Crummey via Openmp-commits]

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

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.


Repository:
  rL LLVM

http://reviews.llvm.org/D14027