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

Improve cmd options #2392

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

KelvinChung2000
Copy link

There is no documentation on the -i switch so I have added a bit of doc about it.

The -i option is a bit clunky, so I have improved the pass option to allow a semi-colon-separated list to specify a pass pipeline.

Finally, the help message listing all the passes in one go is difficult to read. So, I have added a new variable to keep track of the original pass, which can better preserve the help message provided.

@rachitnigam
Copy link
Contributor

The -p option supports this behavior already because you can repeat it (-p p1 -p p2) will run passes p1 and p2 in a sequence.

@KelvinChung2000
Copy link
Author

After implementing the feature, I noticed this was possible. The documentation only implies that it runs a pass and does not mention ordering. However, the order in which each -p is applied is unclear since command-line input does not always indicate order.

@rachitnigam
Copy link
Contributor

However, the order in which each -p is applied is unclear since command-line input does not always indicate order.

I'm not sure I follow. Command-line order is exactly the pass order.

@KelvinChung2000
Copy link
Author

KelvinChung2000 commented Jan 21, 2025

Not all programs necessarily take argument order, parse, and store it in input order.

And I also think that -p "p1;p2" is a lot more concise and clean compared to -p p1 -p p2

src/cmdline.rs Outdated Show resolved Hide resolved
@rachitnigam
Copy link
Contributor

the problem is that now the two styles can be combined and you can write: -p "p1;p2" -p "p3;p4". i would rather there be one uniform way of doing things.

most compiler drivers (llvm-opt, mlir-opt) do actually take passes like this in command-line order.

@KelvinChung2000
Copy link
Author

I would also suggest renaming the insert to reorder and going from -i to -r since it does not insert a pass into the pipeline, as a:b requires both a and b to be in the pipeline in the first place.

@@ -162,7 +165,7 @@ impl PassManager {
});

// Push all aliases
let mut aliases = self.aliases.iter().collect::<Vec<_>>();
let mut aliases = self.aliases_cmd.iter().collect::<Vec<_>>();
Copy link
Contributor

Choose a reason for hiding this comment

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

What does this change help with?

Copy link
Author

Choose a reason for hiding this comment

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

It allows printing the alias pass name instead of the fully expanded pass. So the help message at "all" will be "validate,pre-opt..." instead of the long chain of passes.

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.

2 participants