This is an archive of the discontinued LLVM Phabricator instance.

[OMPT] Handle null pointer in set_callback to improve performance
ClosedPublic

Authored by sconvent on Dec 13 2017, 4:54 AM.

Diff Detail

Repository
rOMP OpenMP

Event Timeline

sconvent created this revision.Dec 13 2017, 4:54 AM
sconvent retitled this revision from Handle null pointer correctly in set_callback to [OMPT] Handle null pointer correctly in set_callback .
Hahnfeld requested changes to this revision.Dec 13 2017, 6:03 AM

Can we have a test for this? Like setting the callback, unsetting it again and checking that the function isn't called?

This revision now requires changes to proceed.Dec 13 2017, 6:03 AM

This change is more a performace optimization. (reset the bit in the bitmap to 0)
The callback was set to NULL before and all callback invocation checks for the NULL-pointer.

In some cases we do not enter a code path if a specific callback is not set. With this patch this code path is also not entered if the callback was set and reset to NULL.

I agree that we might add a testcase, but the testcase will not show different behavior for this patch.

sconvent updated this revision to Diff 126920.Dec 14 2017, 2:50 AM

Added testcase

Can't we shorten this?
I'd prefer

     
ompt_enabled.event_name = (callback != 0);                                           \

to the if which then assigns one or zero.

sconvent updated this revision to Diff 126921.Dec 14 2017, 3:08 AM

Implemented suggestion

This change is more a performace optimization. (reset the bit in the bitmap to 0)
The callback was set to NULL before and all callback invocation checks for the NULL-pointer.

In some cases we do not enter a code path if a specific callback is not set. With this patch this code path is also not entered if the callback was set and reset to NULL.

Then title and summary should say that this is a for improving performace, that's impossible to guess from the changes.

Also, is this test case actually valid? I remember that some callbacks must always be set as pairs, not sure if that's true for parallel though.

runtime/test/ompt/misc/unset_callback.c
8 ↗(On Diff #126921)

Not needed.

sconvent updated this revision to Diff 127847.Dec 21 2017, 2:14 AM
sconvent retitled this revision from [OMPT] Handle null pointer correctly in set_callback to [OMPT] Handle null pointer in set_callback to improve performance.
sconvent edited the summary of this revision. (Show Details)

Removed unnecessary definition.

The parallel_begin and parallel_end callbacks can be set independently by my understanding of the spec. So the testcase should be valid.

sconvent marked an inline comment as done.Dec 21 2017, 2:15 AM
This revision is now accepted and ready to land.Dec 21 2017, 2:38 AM
This revision was automatically updated to reflect the committed changes.
protze.joachim added inline comments.Dec 21 2017, 6:24 AM
runtime/test/ompt/misc/unset_callback.c
8 ↗(On Diff #126921)

Some OpenMP activity is needed to initialize the OpenMP runtime, let the runtime call ompt_start_tool and ompt_inittialize, so that ompt_set_callback is set to non-NULL.

Hahnfeld added inline comments.Dec 21 2017, 7:45 AM
runtime/test/ompt/misc/unset_callback.c
8 ↗(On Diff #126921)

I think Phabricator mangled my comment: I originally meant the unused variable x that Simon removed meanwhile. I agree that we need the first parallel region.