Add PAPI functionality to Dr Hook #26

wdeconinck · 2024-07-24T15:47:06Z

This is cleaned up, updated and rebased PR superseding #18

To enable the DR_HOOK hardware counters, use following environment variables for DR_HOOK-ed programs:
DR_HOOK=1 DR_HOOK_OPT=COUNTERS

The CMake feature DR_HOOK_PAPI is enabled by default when PAPI is found. This can be toggled with
-DENABLE_DR_HOOK_PAPI=ON|OFF. In the case the feature is enabled, then the test fiat_test_drhook_counters is built. For unit-testing purpose, when run as part of ctest, the problem size is very small and the test runs in under 1 second.
This test can also be run standalone:

<build-dir>/tests/fiat_test_drhook_counters

Then the problem size is by default what was chosen by @MartynF for sanity checks.
Upon finalisation the file drhook.prof.0.csv is created which contains all the counter information.

cleanup

Andrew-Beggs-ECMWF

Hi, Ioan asked me to have a look at this PR. I really like it overall, so I hope my harsh review doesn't make it seem like the contrary!

A lot of my comments are either for clarification or style : )

Andrew-Beggs-ECMWF · 2024-08-06T12:31:30Z

src/fiat/drhook/drhook.c

@@ -2882,6 +2931,11 @@ putkey(int tid, drhook_key_t *keyptr, const char *name, int name_len,
  else if (tid >= 1 && tid <= numthreads) {
    double delta_wall = 0;
    double delta_cpu  = 0;
+    long_long * delta_counters=NULL;
+#if defined(DR_HOOK_HAVE_PAPI)
+    delta_counters=alloca(drhook_papi_num_counters() * sizeof(long_long) );


What is the reason for the use of alloca? I'd not come across it before and I'm wondering why it's chosen over malloc_drhook etc : )

Also, will it have an unintended affect on Dr Hook's memory tracking?

Andrew-Beggs-ECMWF · 2024-08-06T12:33:08Z

src/fiat/drhook/drhook.c

@@ -2603,6 +2647,8 @@ process_options()
      else if (strequ(p,"CALLPATH")) {
        opt_callpath = 1;
        OPTPRINT(fp,"%s%s",comma,"CALLPATH"); comma = ",";
+      } else {
+	printf("DrHook: Note - no match for HOOK_OPT : %s\n",p);


Nice QoL improvement!

Andrew-Beggs-ECMWF · 2024-08-06T16:10:05Z

src/fiat/drhook/drhook_papi.c

+/* hardwired for now */
+const char* hookCounters[NPAPICNTRS][2]= {
+   {"PAPI_TOT_CYC","Cycles"},
+   {"PAPI_FP_OPS","FP Operations"},
+   {"PAPI_L1_DCA","L1 Access"},
+   {"PAPI_L2_DCM","L2 Miss"}
+};


I appreciate this is probably not the intended long term solution for specifying counters, but is it worth making this more flexible now? My concern is that a temporary solution will become permanent.

Perhaps a DR_HOOK_PAPI_COUNTERS similar to how DR_HOOK_OPT is done? If not specified it could default to these 4. I am happy to implement this if we want to do this now : )

Andrew-Beggs-ECMWF · 2024-08-06T16:32:21Z

src/fiat/drhook/drhook_papi.c

+/* function to use for thread id 
+   - it should be better than omp_get_thread_num!
+*/
+unsigned long safe_thread_num(){
+  return oml_my_thread()-1;
+}


Should this be here? I'm of the opinion it's either:

Part of PAPI and should be namespaced as such

For general use and doesn't belong in this file

Andrew-Beggs-ECMWF · 2024-08-06T16:53:49Z

src/fiat/drhook/drhook_papi.c

+/* a = b - c  (b or c == NULL means use current readings) */      
+void drhook_papi_subtract(long_long* a, long_long* b, long_long* c){
+  if (b==NULL || c==NULL){
+    long_long * tmp=alloca(drhook_papi_num_counters() * sizeof(long_long));


Similar comments about alloca as above

Andrew-Beggs-ECMWF · 2024-08-07T12:31:59Z

src/fiat/drhook/drhook_papi.c

+  events[thread]=PAPI_NULL;
+  papiErr=PAPI_create_eventset(&events[thread]);
+  if (papiErr != PAPI_OK){
+    snprintf(pmsg,STD_MSG_LEN,"DRHOOK:PAPI: create event set failed (%s) \n",PAPI_strerror(papiErr));


Suggested change

snprintf(pmsg,STD_MSG_LEN,"DRHOOK:PAPI: create event set failed (%s) \n",PAPI_strerror(papiErr));

snprintf(pmsg,STD_MSG_LEN,"DRHOOK:PAPI: Error, create event set failed (%s) \n",PAPI_strerror(papiErr));

Andrew-Beggs-ECMWF · 2024-08-07T12:32:16Z

src/fiat/drhook/drhook_papi.c

+
+    papiErr=PAPI_event_name_to_code(hookCounters[counter][0],&eventCode);
+    if (papiErr != PAPI_OK){
+      snprintf(pmsg,STD_MSG_LEN,"DRHOOK:PAPI: event name to code failed (%s)",PAPI_strerror(papiErr));


Suggested change

snprintf(pmsg,STD_MSG_LEN,"DRHOOK:PAPI: event name to code failed (%s)",PAPI_strerror(papiErr));

snprintf(pmsg,STD_MSG_LEN,"DRHOOK:PAPI: Error, event name to code failed (%s)",PAPI_strerror(papiErr));

Andrew-Beggs-ECMWF · 2024-08-07T12:32:31Z

src/fiat/drhook/drhook_papi.c

+
+    papiErr=PAPI_add_event(events[thread],eventCode);
+    if (papiErr!=PAPI_OK){
+      snprintf(pmsg,STD_MSG_LEN,"DRHOOK:PAPI: add_event failed: %d (%s)",papiErr,PAPI_strerror(papiErr));


Suggested change

snprintf(pmsg,STD_MSG_LEN,"DRHOOK:PAPI: add_event failed: %d (%s)",papiErr,PAPI_strerror(papiErr));

snprintf(pmsg,STD_MSG_LEN,"DRHOOK:PAPI: Error, add_event failed: %d (%s)",papiErr,PAPI_strerror(papiErr));

Andrew-Beggs-ECMWF · 2024-08-07T12:33:45Z

src/fiat/drhook/drhook_papi.c

+  papiErr=PAPI_start(events[thread]);
+
+  if (papiErr != PAPI_OK) {
+    snprintf(pmsg,STD_MSG_LEN,"DRHOOK:PAPI: starting counters failed (%d=%s)",papiErr,PAPI_strerror(papiErr));


Suggested change

snprintf(pmsg,STD_MSG_LEN,"DRHOOK:PAPI: starting counters failed (%d=%s)",papiErr,PAPI_strerror(papiErr));

snprintf(pmsg,STD_MSG_LEN,"DRHOOK:PAPI: Error, starting counters failed (%d=%s)",papiErr,PAPI_strerror(papiErr));

Andrew-Beggs-ECMWF · 2024-08-07T13:12:45Z

src/fiat/drhook/internal/drhook_run_omp_parallel.F90

+  use OML_MOD
+  implicit none
+  INTEGER(KIND=C_INT), INTENT(INOUT) :: Events(n)
+  INTEGER(KIND=C_INT), VALUE :: n


Suggested change

INTEGER(KIND=C_INT), VALUE :: n

INTEGER(KIND=C_INT), VALUE, INTENT(IN) :: n

Should this have an INTENT attribute?

This commit removes all tabs, in favour of two spaces, in files touched by ecmwf-ifs#26 and ecmwf-ifs#27.

wdeconinck requested a review from ioanhadade July 24, 2024 15:49

wdeconinck mentioned this pull request Jul 24, 2024

Add PAPI functionality to Dr Hook #18

Closed

wdeconinck and others added 5 commits July 24, 2024 16:02

CI: update gha runner to macos 13

8a0d7b0

Add DR_HOOK support for HW counters with PAPI

586191a

Make fiat-drhook-sanity a unit-test, refactor CMake and other cosmetic

e3ff319

cleanup

Fix invalid arguments error to PAPI_list_events

bcd9d78

Fix DR_HOOK_SILENT option

374e33a

wdeconinck force-pushed the papi branch from b0206a4 to 374e33a Compare July 24, 2024 16:16

wdeconinck changed the base branch from main to develop July 24, 2024 16:17

Andrew-Beggs-ECMWF suggested changes Aug 7, 2024

View reviewed changes

Andrew-Beggs-ECMWF mentioned this pull request Aug 21, 2024

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

Draft

Andrew-Beggs-ECMWF added a commit to Andrew-Beggs-ECMWF/fiat that referenced this pull request Aug 28, 2024

Fix whitespacing

0cf6b5d

This commit removes all tabs, in favour of two spaces, in files touched by ecmwf-ifs#26 and ecmwf-ifs#27.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add PAPI functionality to Dr Hook #26

Add PAPI functionality to Dr Hook #26

wdeconinck commented Jul 24, 2024

Andrew-Beggs-ECMWF left a comment

Andrew-Beggs-ECMWF Aug 6, 2024

Andrew-Beggs-ECMWF Aug 6, 2024

Andrew-Beggs-ECMWF Aug 6, 2024

Andrew-Beggs-ECMWF Aug 6, 2024

Andrew-Beggs-ECMWF Aug 6, 2024

Andrew-Beggs-ECMWF Aug 7, 2024

Andrew-Beggs-ECMWF Aug 7, 2024

Andrew-Beggs-ECMWF Aug 7, 2024

Andrew-Beggs-ECMWF Aug 7, 2024

Andrew-Beggs-ECMWF Aug 7, 2024

	snprintf(pmsg,STD_MSG_LEN,"DRHOOK:PAPI: create event set failed (%s) \n",PAPI_strerror(papiErr));
	snprintf(pmsg,STD_MSG_LEN,"DRHOOK:PAPI: Error, create event set failed (%s) \n",PAPI_strerror(papiErr));

	INTEGER(KIND=C_INT), VALUE :: n
	INTEGER(KIND=C_INT), VALUE, INTENT(IN) :: n

Add PAPI functionality to Dr Hook #26

Are you sure you want to change the base?

Add PAPI functionality to Dr Hook #26

Conversation

wdeconinck commented Jul 24, 2024

Andrew-Beggs-ECMWF left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment