papi framework: Do not allow events to be added without the component or pmu prefix names unless applicable#528
Conversation
|
I am reviewing this PR. |
|
I tested these changes on the Frontier supercomputer, which contains the AMD Zen3 CPU and AMD MI250X GPU architectures using ROCm 7.0.2. The PAPI utilities (
@Treece-Burgess Can you please address my other review comments and rebase? After you do, I will approve this PR, and we can merge it. Thank you! |
… or pmu prefix names unless applicable
4792295 to
0e8d7c9
Compare
…u i.e. perf:: that a user has to adhere to this when adding the event.
|
I am testing the latest commit. |
|
Testing this on Frontier with (AMD Zen3 CPU architecture), I confirm that the latest commit results in proper behavior for PMU prefixes: In addition, the "default" PMU is still allowed for the applicable native @jagode Since this PR introduces a change (albeit small) to the |
|
Hello @deater, could you take a look at the change I made in As an overview of this change, users will need to add native events for the |
|
I think this is probably a good change, but it is possible it might break people's existing code that depends on the old behavior. I don't know if we'd need a more serious version bump because of that? Related is the issue with the perf component where it has traditionally had the idea of the "default" component (which typically is the cpu component) and you don't need a prefix for events like that. Your changeset here doesn't seem to change that. This is a case where if we forced even the default component to have a prefix it would make things like handling hybrid CPUs a lot easier, however it might break a lot more people's code and require updating the papi_events.csv file, etc |
We internally consider the old behavior as a bug rather than a feature. However, I do know this old behavior has been in the codebase for a while and agree that it most likely will result in some users code possibly breaking. The next release for PAPI will be 7.3.0, by a more serious bump do you mean going up to 8.0.0?
Correct, this PR does not change the behavior of the "default" component cases such as |
|
I don't have a good feel for if this is a big enough break to bump to 8.0 If we were bumping to 8.0 I'd suggest we break some other things at the same time, such as maybe getting rid of the default component. We can probably get rid of default component in steps, maybe just always show the component in papi_native_avail at first but then still handle the lack of component for backwards compat? Things get complicated because technically all of the CPU events (core and uncore) are technically perf events handled by the perf component but it would be a bit much to ask them to have events like perf::hsw_ep::instructions or whatever. |
Pull Request Description
Issue:
Currently in the master branch, users are able to add native events without their component prefix name. For example, the component
appiocontains an eventREAD_BYTESand can still successfully be added if it lacks the component prefix nameappio::::This behavior is undesired as:
papi_native_availdoes show the applicable component or pmu prefix namesappioandiocomponentsThis PR resolves this behavior and closes Issue #526.
Testing
Testing Setup
Testing was done on Methane at ICL with:
Testing Results
appiocomponent tests after addingappio:::to hardcoded event names: ✅examplecomponent tests after addingexample:::to hardcoded event names: ✅* -
papi_component_avail,papi_native_avail,papi_command_lineAuthor 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