[compiler driver] improving pipeline parsing #1519
Open
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Hello. You may have forgotten to update the changelog!
|
erick-xanadu
force-pushed
the
eochoa/2025-02-13/improving-pipeline-parsing
branch
from
d68e5dc to
c22bc18
Compare
erick-xanadu
force-pushed
the
eochoa/2025-02-13/improving-pipeline-parsing
branch
from
c22bc18 to
765267a
Compare
mehrdad2m
reviewed
Feb 20, 2025
There was a problem hiding this comment.
Thanks @erick-xanadu, This looks good. The only comment I have is that this change should be reflected on the catalyst CLI documentation as well.
erick-xanadu
force-pushed
the
eochoa/2025-02-13/improving-pipeline-parsing
branch
from
765267a to
6c854ee
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1519 +/- ##
==========================================
- Coverage 96.08% 96.08% -0.01%
==========================================
Files 81 81
Lines 8615 8614 -1
Branches 816 817 +1
==========================================
- Hits 8278 8277 -1
Misses 280 280
Partials 57 57 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
mehrdad2m
reviewed
Mar 26, 2025
| @@ -212,7 +221,7 @@ pass that is applied, and the ``-o`` option to set the name of the output IR fil | |||
|
|
|||
| catalyst my_circuit.mlir \ | |||
| --tool=opt \ | |||
| --catalyst-pipeline="pipe(remove-chained-self-inverse;merge-rotations)" \ | |||
| --catalyst-pipeline="pipe:builtin(remove-chained-self-inverse,merge-rotations)" \ | |||
There was a problem hiding this comment.
Suggested change
| --catalyst-pipeline="pipe:builtin(remove-chained-self-inverse,merge-rotations)" \ | |
| --catalyst-pipeline="pipe:remove-chained-self-inverse,merge-rotations" \ |
same here and some other places in the docs
| applies the passes ``split-multiple-tapes`` and ``apply-transform-sequence``, and where ``pipe2`` | ||
| applies the pass ``inline-nested-module``, we would specify this pipeline configuration as: | ||
|
|
||
| .. code-block:: | ||
|
|
||
| --catalyst-pipeline="pipe1(split-multiple-tapes;apply-transform-sequence),pipe2(inline-nested-module)" | ||
| --catalyst-pipeline='pipe1:builtin-module(split-multiple-tapes,apply-transform-sequence)' |
There was a problem hiding this comment.
Suggested change
| --catalyst-pipeline='pipe1:builtin-module(split-multiple-tapes,apply-transform-sequence)' | |
| --catalyst-pipeline='pipe1:split-multiple-tapes,apply-transform-sequence' |
I don't think we actually need to have the builtin.module wrapper anymore do we?
This is how the tests are calling the pipeline anyways.
| // RUN: catalyst --tool=opt %s -cse --dump-catalyst-pipeline --verify-diagnostics 2>&1 | FileCheck %s --check-prefix=CHECK-ONE-PASS | ||
| // RUN: not catalyst --tool=opt %s -cse --catalyst-pipeline="pipe1(split-multiple-tapes;apply-transform-sequence),pipe2(inline-nested-module)" --dump-catalyst-pipeline --verify-diagnostics 2>&1 | FileCheck %s --check-prefix=CHECK-FAIL | ||
| // RUN: not catalyst --tool=opt %s -cse --catalyst-pipeline="pipe1:builtin.module(split-multiple-tapes,apply-transform-sequence)" --catalyst-pipeline="pipe2:inline-nested-module" --dump-catalyst-pipeline --verify-diagnostics 2>&1 | FileCheck %s --check-prefix=CHECK-FAIL |
There was a problem hiding this comment.
I guess we have one case with builtin.module here, but this is a case that should fail so it is not actually verifying whether builtin.module is working.
Suggested change
| // RUN: not catalyst --tool=opt %s -cse --catalyst-pipeline="pipe1:builtin.module(split-multiple-tapes,apply-transform-sequence)" --catalyst-pipeline="pipe2:inline-nested-module" --dump-catalyst-pipeline --verify-diagnostics 2>&1 | FileCheck %s --check-prefix=CHECK-FAIL | |
| // RUN: not catalyst --tool=opt %s -cse --catalyst-pipeline="pipe1:split-multiple-tapes,apply-transform-sequence" --catalyst-pipeline="pipe2:inline-nested-module" --dump-catalyst-pipeline --verify-diagnostics 2>&1 | FileCheck %s --check-prefix=CHECK-FAIL |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Context: MLIR uses pipelines, Catalyst also uses pipelines. However, unlike MLIR pipelines, Catalyst's pipelines have names. This makes the syntax for denoting pipelines a little bit different.
Description of the Change: Make the syntax match better. Use multiple --catalyst-pipeline commands to denote multiple pipelines. Detects whether async is necessary by checking the passes instead of passing it through the command line.
Benefits: Avoid custom parsing. Have the same syntax for MLIR pipelines and Catalyst pipelines. Unlocks the possibility of multithreading compilation by using the regular pipeline syntax to denote function level passes.
Possible Drawbacks:
Related GitHub Issues: