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

Support for the ICV nthreads-var in ompd_enumerate_icvs(), ompd_get_icv_from_scope() and ompd_get_icv_string_from_scope() #73

Merged
merged 5 commits into from
Jan 23, 2020

Conversation

jinisusan
Copy link

Review and pull request for providing support for the ICV nthreads-var. (#72).

Thanks!
Jini.

@jinisusan
Copy link
Author

Gentle reminder !

Thanks,
Jini

@jprotze
Copy link

jprotze commented Dec 16, 2019

As I read the spec, ompd_get_icv_from_scope should always just provide a single integer in icv_value for which the tool provides the storage. If that is not possible, the function can return ompd_rc_incompatible.
In this case, the tool would need to use and parse ompd_get_icv_string_from_scope.

For ompd-nthreads-var this means, the function can return a value if the variable is just a single value, but ompd_rc_incompatible for nested execution with a list of values.

Does this make sense?

@jinisusan
Copy link
Author

Thank you very much, Joachim, for pointing this out. You are right -- this makes sense. Will send an updated revision.

@jinisusan
Copy link
Author

Hi again Joachim,

As I thought more about this, I feel that it might make more sense for the debugger to directly invoke ompd_get_icv_string_from_scope() for the ICVs like nthreads-var where the value of the ICV is a list (even if it constitutes of only a single element). This would avoid the (possibly) unnecessary calls from the debugger to ompd_get_icv_from_scope() for the cases where the value is a list of multiple elements. What do you think ? I am planning to ask about this in the omp-tools forum also.

Thanks,
Jini.

…cv_from_scope() and ompd_icv_get_string_from_scope()
…cv_from_scope() and ompd_icv_get_string_from_scope()
@jinisusan
Copy link
Author

Have committed changes according to Joachim's and John's suggestions in omp-tools. Please take a look.

Thanks!
Jini

@jinisusan
Copy link
Author

Gentle reminder ! Thanks !

@jinisusan jinisusan changed the title Support for the ICV nthreads-var in ompd_enumerate_icvs() and ompd_get_icv_from_scope() Support for the ICV nthreads-var in ompd_enumerate_icvs(), ompd_get_icv_from_scope() and ompd_get_icv_string_from_scope() Jan 23, 2020
@jinisusan
Copy link
Author

Made commits to replace an assert call with a ompd_rc_error return in the case of the callback table not being available (as per the review comment in #75 )

ompd_rc_incompatible is returned. The tool can check the return value and
can choose to invoke ompd_get_icv_string_from_scope() if needed. */
if (current_nesting_level < used - 1) {
return ompd_rc_incompatible;
Copy link

Choose a reason for hiding this comment

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

Note for OMPD specification: we might add a new return code: ompd_rc_incomplete.
For the currently available return codes, I would not expect meaningful values unless ompd_rc_ok.

@jprotze jprotze merged commit 383801b into OpenMPToolsInterface:ompd-tests Jan 23, 2020
@jinisusan
Copy link
Author

Thanks, Joachim!

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.

2 participants