ROCm / rocprofiler-compute

Advanced Profiling and Analytics for AMD Hardware
https://rocm.docs.amd.com/projects/omniperf/en/latest/
MIT License
135 stars 49 forks source link

'-k' flag does not respect '--max-kernel-num' #187

Closed skyreflectedinmirrors closed 1 year ago

skyreflectedinmirrors commented 1 year ago

Describe the bug

I want to analyze the 11th most expensive kernel in my application. First I was confused why --list-kernels didn't go past 10, but then I realized there is a --max-kernel-num option to extend this list. However, even with this applied, I cannot actually select anything past 10 to analyze:

$ omniperf analyze -p workloads/<foo>/mi200/ --max-kernel-num 30 -k 16
--------
Analyze
--------

16 is an invalid kernel filter. Must be between 0-9.

Development Environment:

To Reproduce Steps to reproduce the behavior:

  1. Profile any workload with > 10 kernels
  2. Run analyze on e.g., the 11th kernel

Expected behavior

IMO, we should:

  1. Make the --list-kernels option ignore the max kernel number. If a user is asking for the full list of kernels, they probably want the whole thing, and not to see only the top 10.
  2. If the user is asking for a specific kernel (e.g., -k 11), just give it to them (or report that it's out of bounds), rather than limiting it to --max-kernel-num

Essentially --max-kernel-num should only be used for expanding the number of kernels under consideration for analyzing and displaying stats over many kernels.

skyreflectedinmirrors commented 1 year ago

https://github.com/AMDResearch/omniperf/blob/cc5bba19f4ed60310371ca2cea0980f461614f9b/src/omniperf_analyze/omniperf_analyze.py#L303

right now the analyze check is hard-coded here^. I was able to verify that changing the check to >= args.max_kernel_num at least let me see > 10 kernels, but that doesn't address the "expected" behavior

coleramos425 commented 1 year ago

Thanks for the suggestion. I agree 100%. I've made the relevant changes and functionality now mirrors that which you outlined in "Expected behavior"