[OpenMP][Tool] Header-only multiplexing of OMPT tools

[protze.joachim]

Summary

The ompt-multiplex.h header allows to use multiple OMPT tools at a time.
Example usage is given in the tests directory.

Details on usage in README.md

[jdoerfert]

openmp/tools/multiplex/ompt-multiplex.h

12

1. Can we make the file header the same as LLVM files?
2. Can we include the idea and short usage + link to the README in the file doxygen comment?

100

Why do we have commented lines here?

319

Are we sure we don't want to generate these as well?

764

Style: Given that the file is new, please clang format it.

900

I was unable to determine how this macro helps, e.g., why is it better than OMPT_MULTIPLEX_H?

[protze.joachim]

Addressed the comments from Johannes.
Also applied clang-format to all new files.

Increased reuse of callback.h for the test file custom_data_storage.c.first.tool.c

[protze.joachim]

openmp/tools/multiplex/ompt-multiplex.h

12

I used the same header as any file in openmp/runtime.
I updated the header as requested.

100

Fair point. I added the implementation for those functions. We cannot test them because these callbacks are not yet implemented in the runtime (mostly libomptarget)

319

The challenge in generating those functions is the ompt_multiplex_get_own_task_data / ompt_multiplex_get_client_task_data, which must be applied to all *_data arguments. Such arguments are all over the place.

764

This part was actually clang-formated.
By moving the ; to the definition of OMPT_LOAD_CLIENT_FOREACH_OMPT_EVENT, clang-format now also accepts that definition.

900

This macro renames the ompt_start_tool function of the original tool to ompt_multiplex_own_start_tool.
The multiplex header defines ompt_start_tool in line 846, which calls the renamed function in line 888.

[jdoerfert]

openmp/tools/multiplex/tests/print/print.c.second.tool.c

1 ↗

(On Diff #268947)

I don't understand these run lines.

[jdoerfert]

Accidentally accepted before.

[protze.joachim]

lit would warn about missing run lines for these C files, implementing the tool libraries.
All three files in each directory are built and used by the test application which is print.c in this case.

I think, I can rename the test application to suffix .app.c and change the test suffix to this.

[protze.joachim]

Ok, this is not how suffix works in lit :(
lit splits file suffix using base,ext = os.path.splitext(filename). This will always be .c for C files. We could play tricks with non-standard file suffixes for the libraries built and used for the test. The extension is then compared to the list provided in config.suffixes.

Just for reference, if I remove the RUN line, the output of make check-ompt-multiplex will contain this:

UNRESOLVED: OMPT multiplex :: custom_data_storage/custom_data_storage.app.c.second.tool.c (1 of 6)
******************** TEST 'OMPT multiplex :: custom_data_storage/custom_data_storage.app.c.second.tool.c' FAILED ********************
Test has no run line!
********************

I could change the lines to:

// RUN: do-not-run
// REQUIRES: do-not-run

But then we always have 2 successful, 4 unsupported tests listed instead of 6 successful tests.

[protze.joachim]

As an alternative, I could make the tools header files, which are conditionally included:

#ifdef FIRST
#include <first_tool.h>
#elif SECOND
#include <second_tool.h>
#else
... current app content
#endif

Then we have only one C file, which has the app code and the test expressions. The tool libraries are compiled from the header files.

[jdoerfert]

In D76012#2083276, @protze.joachim wrote:

As an alternative, I could make the tools header files, which are conditionally included:

#ifdef FIRST
#include <first_tool.h>
#elif SECOND
#include <second_tool.h>
#else
... current app content
#endif

Then we have only one C file, which has the app code and the test expressions. The tool libraries are compiled from the header files.

Assuming I understand this properly, this makes sense. Some places we also have test/Inputs/ folders for "include headers" but if they can live right in the print folder, fine with me.

[protze.joachim]

The tool libraries are now implemented in include files as suggested in my last comment.

[protze.joachim]

Update the patch to use _TOOL_PREFIX instead of TOOL_PREFIX and undef after the include.

[jdoerfert]

As there seem to be no concerns, LGTM.

[sylvestre.ledru]

@protze.joachim @jdoerfert ompt-multiplex.h is installed by default in /usr/include/
others opemp headers are installed in /usr/include/openmp/

is that by design here?

[sylvestre.ledru]

maybe this improvement should be added to the release notes too ?

[jdoerfert]

In D76012#2100421, @sylvestre.ledru wrote:

@protze.joachim @jdoerfert ompt-multiplex.h is installed by default in /usr/include/
others opemp headers are installed in /usr/include/openmp/

is that by design here?

I didn't catch that. I doubt it is, @protze.joachim, right?

Re change notes:
Yes, this is a good one. I always forget.
^ @protze.joachim can u make a patch?

[protze.joachim]

Where are OpenMP release notes supposed to be added? I'm surprised, that I did not find any OpenMP related notes in the repository.

[Hahnfeld]

In D76012#2100421, @sylvestre.ledru wrote:

@protze.joachim @jdoerfert ompt-multiplex.h is installed by default in /usr/include/
others opemp headers are installed in /usr/include/openmp/

Which options do you use to get the headers to include/openmp/? I think this must be something related to your CMake invocation because for example https://www.archlinux.org/packages/extra/x86_64/openmp/ has it in /usr/include/.

[jdoerfert]

In D76012#2102755, @protze.joachim wrote:

Where are OpenMP release notes supposed to be added? I'm surprised, that I did not find any OpenMP related notes in the repository.

I think we do not have any, yet. Maybe we should. Could you create a file matching other subprojects?