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

Only evaluate metrics in analyze for architectures we're actually going to use! #144

Closed
skyreflectedinmirrors opened this issue Jun 30, 2023 · 3 comments
Assignees
Labels
enhancement New feature or request

Comments

@skyreflectedinmirrors
Copy link
Contributor

We know that the python parsing of metrics has high overhead on each analysis / GUI run.
However, I did not know that we are parsing&compiling metrics for all available gfx90X architectures each analyze run.
I discovered this while modifying some of the yaml files, wherein I discovered that I had a syntax error in one of my modified config files.
I fixed it in gfx90a only, to test the change, but was quite surprised when I kept hitting the same syntax error!

Backtracing in pdb lead me to:

https://github.com/AMDResearch/omniperf/blob/a346db7646b0a935f4cac51d131b4a585f065c05/src/omniperf_analyze/omniperf_analyze.py#L97

where-in I realized, we're compiling for all architectures:

p archConfigs.keys()
dict_keys(['gfx906', 'gfx908', 'gfx90a'])

A quick optimization (that will lower the overhead of analyze by ~3x) is to detect which architecture we're going to analyze (at the very least, we have access to the miXXX part of the --path the user passed in, but it's also in the sysinfo.csv file in said path), and only parse&compile metrics for that arch.

@coleramos425
Copy link
Collaborator

Things are a bit more complicated since we support baseline comparison functionality in CLI. If I'm conducting an intra-soc comparison we'd need ArchConfigs to be set for each architecture, i.e.

$ omniperf analyze -p workloads/test_1/mi200/ -p workloads/test_2/mi100/

For now, I'm just detecting the "newest" architecture loaded in ArchConfigs and using this to form the baseline comparison data tables. This should work in the short term considering newer archs should contain a superset of all tables avail in older archs.

May want to revisit this in the "Omniperf Revamp"...

coleramos425 added a commit that referenced this issue Aug 2, 2023
Signed-off-by: coleramos425 <colramos@amd.com>
@skyreflectedinmirrors
Copy link
Contributor Author

skyreflectedinmirrors commented Aug 2, 2023 via email

@coleramos425
Copy link
Collaborator

Merged into dev. Closing issue.

coleramos425 added a commit that referenced this issue Aug 15, 2023
Signed-off-by: coleramos425 <colramos@amd.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants