Skip to content

Implement performance checking script: i8 vs f16 non-fused conv kernels #1727

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

Closed
wants to merge 10 commits into from

Conversation

dorde-antic
Copy link
Contributor

Implement shell script that captures the performance difference between data types to validate expected kernel performance.
Resolves ROCm/rocMLIR-internal#1674

@dorde-antic dorde-antic marked this pull request as ready for review January 28, 2025 16:57
@dorde-antic dorde-antic requested a review from causten as a code owner January 28, 2025 16:57
@dorde-antic dorde-antic marked this pull request as draft January 28, 2025 16:59
@dorde-antic dorde-antic marked this pull request as ready for review January 28, 2025 17:38
Copy link

codecov bot commented Feb 4, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #1727      +/-   ##
===========================================
- Coverage    78.52%   77.99%   -0.52%     
===========================================
  Files          100      100              
  Lines        29907    30057     +150     
  Branches      4452     4656     +204     
===========================================
- Hits         23482    23442      -40     
- Misses        4590     4601      +11     
- Partials      1835     2014     +179     
Flag Coverage Δ
mfma 77.98% <ø> (-0.54%) ⬇️
navi3x 77.98% <ø> (?)
navi4x 77.99% <ø> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

see 34 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@causten causten requested a review from djramic February 24, 2025 16:10
Copy link
Contributor

@djramic djramic left a comment

Choose a reason for hiding this comment

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

LGTM

@dorde-antic dorde-antic force-pushed the performance-checking branch from 6fe114d to 896b225 Compare April 7, 2025 09:55
@dorde-antic dorde-antic force-pushed the performance-checking branch from 896b225 to dad50ef Compare May 14, 2025 13:58
@dorde-antic dorde-antic force-pushed the performance-checking branch from 45fd26f to 520da0c Compare May 19, 2025 14:11
@dorde-antic dorde-antic requested a review from dhernandez0 June 19, 2025 10:09
@dorde-antic dorde-antic changed the title Implement performance checking script Implement performance checking script: i8 vs f16 non-fused conv kernels Jun 19, 2025
@causten causten requested a review from Copilot June 23, 2025 14:21
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR introduces a shell script to benchmark and compare FP16 vs INT8 performance for non-fused convolution kernels.

  • Adds perf-test-i8-f16-conv.sh to compile, time, and record average runtimes for both data types.
  • Supports configurable model name, ONNX path, and iteration count via flags.
  • Outputs results into a directory named after the model with a times summary file.
Comments suppressed due to low confidence (1)

mlir/utils/performance/perf-test-i8-f16-conv.sh:5

  • The usage comment references /performance-checking instead of this script's actual name (perf-test-i8-f16-conv.sh); updating it will avoid confusion.
# Usage: /performance-checking --d <model> --p <model_path> [--r <number_of_iterations>]"

Signed-off-by: Djordje Antic <[email protected]>
total_time=0

compiled="$testcase.mxdb"
migraphx-driver compile "$testcase" --mlir -o "$compiled" > /dev/null
Copy link
Member

@umangyadav umangyadav Jun 24, 2025

Choose a reason for hiding this comment

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

Why this script is in rocMLIR ? Looks like better place is inside migraphx-benchmark-utils
https://github.com/ROCm/migraphx-benchmark-utils

Copy link
Contributor Author

@dorde-antic dorde-antic Jun 24, 2025

Choose a reason for hiding this comment

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

On MLIR Standup (in january) we talked to put it in /mlir/utils/performance. But I agree with what you have said. Should I open PR there and put this script then? @umangyadav

Copy link
Contributor Author

Choose a reason for hiding this comment

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

And would it be in this directory: https://github.com/ROCm/migraphx-benchmark-utils/tree/main/scripts and to add it to readme table? @umangyadav

Copy link
Member

Choose a reason for hiding this comment

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

You can close this one

@dorde-antic
Copy link
Contributor Author

Merged on migraphx-benchmark-utils repo - https://github.com/ROCm/migraphx-benchmark-utils/pull/70

@dorde-antic dorde-antic deleted the performance-checking branch June 24, 2025 14:52
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.

4 participants