-
Notifications
You must be signed in to change notification settings - Fork 49
Move CSE out of MIGraphXToTosaPass #2012
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome, thanks!
Nightly run passed on MI325: https://ml-ci-internal.amd.com/job/MLIR/job/mlir/job/PR-2012/3/pipeline-overview |
There was a problem hiding this 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 moves the Common Subexpression Elimination (CSE) pass out of the MIGraphXToTosaPass implementation and into the MIGraphX pipeline to improve debugging clarity with mlir-print-ir-after-all
.
- Removes nested
runPipeline()
call from MIGraphXToTosaPass that was causing confusing IR output - Adds CSE as a separate pass in the MIGraphX pipeline after the TOSA conversion
- Updates test files to include CSE in their pass sequences
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
File | Description |
---|---|
mlir/test/rocmlir-driver/pipelines.mlir | Updates expected pipeline output to include CSE pass |
mlir/test/Conversion/MIGraphXToTosa/mixr-to-tosa-ops.mlir | Adds CSE pass to test RUN command |
mlir/lib/Dialect/MIGraphX/Pipeline/Pipeline.cpp | Adds CSE pass to the high-level MIGraphX pipeline |
mlir/lib/Conversion/MIGraphXToTosa/MIGraphXToTosaPass.cpp | Removes nested CSE execution from MIGraphXToTosaPass |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
This is passing the nightly CI run here: https://ml-ci-internal.amd.com/job/MLIR/job/mlir/job/PR-2012/8/pipeline-overview/. There seems to be an issue where there are currently no gfx942 nodes in the CI which is why this is not finishing. |
Motivation
Currently, CSE is invoked from within MIGraphXToTosaPass and it causes some confusion when looking at the output of
mlir-print-if-after-all
as it appears that CSE is doing the conversion from MIGraphX -> TOSA.This implements: https://github.com/ROCm/rocMLIR-internal/issues/2012
Technical Details
Within MIGraphXToTosaPass we were using a nested
runPipeline()
call to invoke CSE after all of the logic for the MIGraphX -> Tosa conversion had taken place. When we went to usemlir-print-ir-after-all
the pass instrumentation correctly prints the IR after each pass, but the nestedrunPipeline()
triggers the IR printing mechanism, which dumps the IR after the CSE pass (which is running on the converted TOSA IR). Once this nested call torunPipeline()
finished it would then recognize that MIGraphXToTosa was complete, and then would print the final IR.The proper thing to do here is move running CSE out into the MIGraphX pipeline.
After this change, when running
-mlir-print-ir-after-all
we now get the following:Test Plan
Test Result
Submission Checklist