Add ability for users to specify PAPI events in Dr Hook #27
+1,930
−213
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
|
|
That looks all like good improvements. Thanks @Andrew-Beggs-ECMWF for doing the review + modifications of #26 . Ultimately you (or someone else?) that looks at benchmarking IFS should be happy with this feature and sign off that everything works as expected. |
Andrew-Beggs-ECMWF
added a commit
to Andrew-Beggs-ECMWF/fiat
that referenced
this pull request
Aug 28, 2024
This commit removes all tabs, in favour of two spaces, in files touched by ecmwf-ifs#26 and ecmwf-ifs#27.
|
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. |
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.
This commit removes all tabs, in favour of two spaces, in files touched by ecmwf-ifs#26 and ecmwf-ifs#27.
|
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 : ) |
Andrew-Beggs-ECMWF
force-pushed
the
papi
branch
from
0cf6b5d to
b19fdd2
Compare
# Conflicts: # src/fiat/drhook/drhook.c
While PAPI code was behind #ifdefs, opt_papi was only used for toggling the printing of counters. PAPI would still be running as part of DrHook. To be inline with other options, PAPI is now behind both #ifdefs statically and if (opt_papi) dynamically.
The qualifier needed to be removed so that malloc_drhook() could be used from within drhook_papi.c. To try and retain some restricted scoping (albeit no longer enforced by the compiler), malloc_drhook() hasn't been added to a header and extern is needed to use it.
Also fixed some related white spacing
Previously nproc was initialised to 0. This meant that get_mon_out() wouldn't work as expected when not using MPI due to a bad assumption in an if statement.
wdeconinck
reviewed
Oct 28, 2024
This reverts commit 8a558aa.
When outputting the value of DR_HOOK_PAPI_COUNTERS, the info prefix was missing.
dr_hook_procinfo() now separates the setting of myproc & nproc from the reporting of initialising MPI. It will now initialise both myproc and nproc to 1 if DrHook is not running with MPI. To report the state of MPI, the new parameter mpi_init will be used.
Tests that any counters over MAXNPAPICNTRS are silently dropped.
Previously set_tests_properties() would fail because it couldn't find fiat_test_drhook_papi_mpi_valid_csv to add properties to. This was because fiat_test_drhook_papi_mpi_valid_csv was behind a condition that required MPI, while set_tests_properties() was not.
Prevents errors of not finding a test when trying to set properties.
Used to ensure type safety between C and Fortran for DR_HOOK_PROCINFO()
Tests were accidentally searching for the wrong string in the regex check
wdeconinck
approved these changes
Feb 6, 2025
There was a problem hiding this comment.
I think this is good.
Perhaps this can be rebased on latest develop, and then I'd like @ioanhadade to test this too.
Previously papi.h was needed by drhook_papi.h, even when building without PAPI. Now it is only needed when PAPI is requested as the type is defined locally.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.