papi_hl region-based calculations#529
Conversation
Treece-Burgess
added
type-feature
src/high-level/papi_hl.c
Outdated
| } | ||
|
|
||
| // except from nvml gpu_inst_power average | ||
| if( (strstr(requested_event_names[i], "nvml:::") != NULL) && (strstr(requested_event_names[i], "gpu_inst_power") != NULL) ) { |
There was a problem hiding this comment.
Q: As you are wanting to calculate a region average for event type 2, should gpu_inst_power actually be gpu_memory_avg_power?
There was a problem hiding this comment.
thank you. I included gpu_memory_avg_power for region-based average, too, for higher accuracy.
src/high-level/papi_hl.c
Outdated
| return ( retval ); | ||
| } | ||
| /* get cycles for last component */ | ||
| retval = PAPI_read_ts( _local_components[num_of_components - 1].EventSet, _local_components[num_of_components - 1].values, &_local_cycles ); |
There was a problem hiding this comment.
Q: Why is the change to this section needed? It does not appear to me that it is behaving correctly.
For instance, if I look solely at gpu_memory_avg_power between the master branch and your branch this event remains as an instant event type. However, in the master branch I get a value of 3472328295419250878, but in your branch I get a value of 35295. If I take your branch and change this section back to how it is in master then I get a comparable value again.
There was a problem hiding this comment.
Exactly. This number: 3472328295419250878 does not look like a valid number. For example gpu_memory_avg_power represents power in mW. Something like that: 18198 mW or 35295 mW, makes more sense to be reported.
There was a problem hiding this comment.
The issue with this change is that the last component will have two reads occur for it which are:
PAPI_readPAPI_read_ts
The latter will overwrite the first PAPI_read value. When a user calls PAPI_hl_region_begin, PAPI_hl_read, or PAPI_hl_region_end they would expect a single read to occur similar to if they called the low level API with PAPI_read. Therefore, this change needs to be reverted back to what it is currently in the master branch.
The reason for this extremely large value is because of the internal code introduced in PR #525 specifically the following block. The value returned is a field_value->value.ullVal; however, for a fieldId of NVML_FI_DEV_POWER_INSTANT you need to use a valueType of uiVal.
I have tested the code in PR #564 with this current PR and I have not seen an extremely large value output after 10 runs.
There was a problem hiding this comment.
I see. changes reverted! thank you!!
|
Hello @Treece-Burgess. Do we have any news on this? |
Hello @mpatrou I will do a final run through of the PR either tomorrow or this weekend. |
2 participants
Pull Request Description
Sibling PR (2/2) split from #525
It updates papi_hl:
Author Checklist
Why this PR exists. Reference all relevant information, including background, issues, test failures, etc
Commits are self contained and only do one thing
Commits have a header of the form:
module: short descriptionCommits have a body (whenever relevant) containing a detailed description of the addressed problem and its solution
The PR needs to pass all the tests