This is an archive of the discontinued LLVM Phabricator instance.

[OMPT] Make sure that OMPT is enabled in runtime entry points that access internals of the runtime
ClosedPublic

Authored by sconvent on Jun 4 2018, 7:11 AM.

Diff Detail

Event Timeline

sconvent created this revision.Jun 4 2018, 7:11 AM
Hahnfeld added inline comments.Jun 4 2018, 7:26 AM
runtime/src/ompt-general.cpp
458

Should we put an if (ompt_enabled.enabled) return ompt_get_callback_failure at the very beginning of the function? That seems invariant to the switch-case...

runtime/src/ompt-specific.cpp
198

Is this needed in internal functions or does it just make things easier? I'm worried that adding these checks to internal functions will degrade performance...

sconvent updated this revision to Diff 150112.Jun 6 2018, 5:07 AM

Address Jonas' comments:

sconvent added inline comments.Jun 6 2018, 5:08 AM
runtime/src/ompt-general.cpp
458

You meant if (!ompt_enabled.enabled) right?

Hahnfeld added inline comments.Jun 6 2018, 5:12 AM
runtime/src/ompt-general.cpp
458

Sure :D

runtime/src/ompt-specific.cpp
235

I think you do the same for the other functions in this file as well.

334

Same here

sconvent updated this revision to Diff 150690.Jun 11 2018, 2:12 AM

Move ompt_enabled.enabled checks out of internal functions

sconvent updated this revision to Diff 151153.Jun 13 2018, 6:59 AM

Add check to ompt_get_state()

sconvent updated this revision to Diff 151687.Jun 18 2018, 4:38 AM

Add a testcase that calls the runtime entry points when OMPT is not enabled.

jmellorcrummey requested changes to this revision.Jun 18 2018, 4:53 AM
jmellorcrummey added a subscriber: jmellorcrummey.

None of this makes sense. If OMPT is not enabled, there should be no call to the ompt_initializer. For that reason, no tool should get pointers to OMPT runtime entry points if OMPT is not enabled. Therefore, the OMPT runtime entry points don’t need to check if OMPT is enabled.

This revision now requires changes to proceed.Jun 18 2018, 4:53 AM

The OMPT tool can decide at multiple points to be inactive, here we look at:

4.2.1.2 Initializing an OMPT Tool

If a tool initializer returns a non-zero value, the tool will be activated for the execution; otherwise, the tool will be inactive.

This tool behavior is represented in line 136 of the test-code. Similar behavior is also expect after the ompt_finalize_tool is added: the tool should be inactive, but the runtime should still not crash or reboot if the tool calls some of the runtime entry points after calling ompt_finalize_tool.

Commenting from my phone without a spec (flying without a net): I think we should change the name of the bit ompt_enabled.enabled to ompt_enabled.activated. That would make joachim’s distinction clear. I was commenting on the setting of OMP_TOOL=enabled. There is a difference between enabled and activated. Simon’s commentary in and about the test case muddled the activated vs. enabled distinction.

I think we should change the name of the bit ompt_enabled.enabled to ompt_enabled.activated. That would make joachim’s distinction clear. I was commenting on the setting of OMP_TOOL=enabled. There is a difference between enabled and activated.

I think, we should first make this distinction clear in the OpenMP spec. Then we can change the name of this bit.

I would like to commit this patch before the 7.0 branch.
Changing the name of the bit can be done in a later patch.
@jmellorcrummey do you agree, can you remove your request for changes?

This revision was not accepted when it landed; it landed in state Needs Revision.Jan 16 2019, 1:02 AM
This revision was automatically updated to reflect the committed changes.

The patch in general looks good to me.

If the name of the variable is an issue, I look forward for a patch fixing this name :)