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 PAPI functionality to Dr Hook #18

Closed
wants to merge 20 commits into from
Closed

Conversation

ioanhadade
Copy link
Collaborator

No description provided.

@FussyDuck
Copy link

FussyDuck commented Mar 5, 2024

CLA assistant check
All committers have signed the CLA.

int number;
int j=1;
#ifdef _OPENMP
j=safe_thread_num();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

safe_thread_num() normally returns 0 when _OPENMP is not defined.
So I thought it seems redundant to have j=safe_thread_num() wrapped with #ifdef _OPENMP.
However then the default value j=1 would become j=0.
That would bypass the further j==k condition when nthreads=1 as is the case without _OPENMP defined.
Is that the intention, why j=1?

@wdeconinck
Copy link
Collaborator

@wdeconinck
Copy link
Collaborator

wdeconinck commented Mar 6, 2024

@MartynF any occurrence of long_long should be replaced with long long. See https://github.com/icl-utk-edu/papi/blob/master/src/papi.h#L554-L559

@MartynF
Copy link

MartynF commented Mar 7, 2024

Did fiat-print-bindings require OpenMP in C? It looks like it has pragmas!

@MartynF
Copy link

MartynF commented Mar 7, 2024

@MartynF any occurrence of long_long should be replaced with long long. See https://github.com/icl-utk-edu/papi/blob/master/src/papi.h#L554-L559

In version 7.... which isn't distributed with majority use OS's including current HPC right now ....

@wdeconinck
Copy link
Collaborator

Did fiat-print-bindings require OpenMP in C? It looks like it has pragmas!

Yes it does, but this is a standalone executable that does not even link to fiat.

In version 7.... which isn't distributed with majority use OS's including current HPC right now ....

Right let's keep it then. Thanks for clarifying.

@ioanhadade
Copy link
Collaborator Author

I would like to get this merged as it has been a while.
What is the holdup @wdeconinck?

@wdeconinck
Copy link
Collaborator

I would like to get this merged as it has been a while. What is the holdup @wdeconinck?

I will take this PR up further now.

@wdeconinck
Copy link
Collaborator

@ioanhadade @MartynF I have created a new PR #26 from the latest develop branch. Please check it still satisfies, and let's continue discussion there. I will close this PR at the moment. Thanks @MartynF for this nice feature!

@wdeconinck wdeconinck closed this Jul 24, 2024
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.

4 participants