Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add ability for users to specify PAPI events in Dr Hook #27

Draft
wants to merge 5 commits into
base: papi
Choose a base branch
from

Conversation

Andrew-Beggs-ECMWF
Copy link

This PR extends #26 by allowing users to specify their own PAPI events (up to MAXNPAPICNTRS, defaulting to 4). This is done with the DR_HOOK_PAPI_COUNTERS flag. If the flag is not specified, and PAPI is enabled, then the following defaults will be chosen:
PAPI_TOT_CYC
PAPI_FP_OPS
PAPI_L1_DCA
PAPI_L2_DCM

If an invalid event is chosen, then DrHook will simply crash will an appropriate error message.

Other commits are also included, which tidy up error messages introduced in #26.

Currently in draft mode to allow for review while adding tests.

This new print clarifies what the next set of prints are referring to.

It also slightly optimises the subsequent control flow.
This allows user to specify their own PAPI events (up to MAXNPAPICNTRS, defaulting to 4).
This is done with the DR_HOOK_PAPI_COUNTERS flag. If the flag is not specified, and PAPI
is enabled, then the following defaults will be chosen:
PAPI_TOT_CYC
PAPI_FP_OPS
PAPI_L1_DCA
PAPI_L2_DCM

If an invalid event is chosen, then DrHook will simply crash will an appropriate error message.
@FussyDuck
Copy link

FussyDuck commented Aug 21, 2024

CLA assistant check
All committers have signed the CLA.

@wdeconinck
Copy link
Collaborator

That looks all like good improvements. Thanks @Andrew-Beggs-ECMWF for doing the review + modifications of #26 .
Only remark I have is to watch the whitespacing for some of the added lines.

Ultimately you (or someone else?) that looks at benchmarking IFS should be happy with this feature and sign off that everything works as expected.

This commit removes all tabs, in favour of two spaces, in files touched by ecmwf-ifs#26 and ecmwf-ifs#27.
@Andrew-Beggs-ECMWF
Copy link
Author

That should be all the tabs banished : )

I found that CMake tests aren't quite powerful enough to do the tests that I was envisioning. So I think you're right that it might just be a case of a "works on my machine" guarantee, which should be fine as I've tested it on our hpc2020 machine.

@Andrew-Beggs-ECMWF
Copy link
Author

Just adding a comment that I intend to do some work to this PR to better align it with how extensions are handled in #28. After that I think it should be good to go : )

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants