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

Handle the options passed to llvm opt and llvm llc #2955

Merged
merged 5 commits into from
Sep 27, 2024

Conversation

chentong319
Copy link
Collaborator

@chentong319 chentong319 commented Sep 25, 2024

Issue #2950 discovered the issue in passing options llvm. There are several problems:

  1. The string of the options has to be split into vector of strings for the command format.
  2. Conflict in options. So far, the optimization level is the only example. The level specified with particular step should have higher priority than that for the whole compiler. Thus, I can avoid multiple "-O#" error.
  3. modify the comments for mllvm, xopt, and xllc to make it easy for user to pass multiple values to these options

I tested with some examples.

@tungld
Copy link
Collaborator

tungld commented Sep 26, 2024

Options of -mllvm is for both llvm opt and llc, but their options are not the same. New option, -mllvmopt and -mllvmllc, are introduced to pass specific option to opt or llc respectively.

Look like -mllvmopt and -mllvmllc are similar to two existing options -Xopt and -Xllc.

Signed-off-by: chentong319 <chentong@us.ibm.com>
@chentong319
Copy link
Collaborator Author

Options of -mllvm is for both llvm opt and llc, but their options are not the same. New option, -mllvmopt and -mllvmllc, are introduced to pass specific option to opt or llc respectively.

Look like -mllvmopt and -mllvmllc are similar to two existing options -Xopt and -Xllc.

That's true. The only difference is that each value of opt and xllc has the same limitation of mllvm: only one option is allowed. I removed the new llvmopt and llvmllc, and modified the option explanation for opt and xllc.

Copy link
Collaborator

@AlexandreEichenberger AlexandreEichenberger left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for the very clear text in the options, with examples. That should be a model for future options!

Copy link
Collaborator

@tungld tungld left a comment

Choose a reason for hiding this comment

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

LGTM.

@chentong319 chentong319 merged commit 2570159 into onnx:main Sep 27, 2024
7 checks passed
@chentong319 chentong319 deleted the multiple-mllvm branch September 27, 2024 17:11
@jenkins-droid
Copy link
Collaborator

Jenkins Linux amd64 Build #15721 [push] Handle the options passe... started at 12:13

@jenkins-droid
Copy link
Collaborator

Jenkins Linux s390x Build #15724 [push] Handle the options passe... started at 13:13

@jenkins-droid
Copy link
Collaborator

Jenkins Linux ppc64le Build #14751 [push] Handle the options passe... started at 13:24

@jenkins-droid
Copy link
Collaborator

Jenkins Linux amd64 Build #15721 [push] Handle the options passe... passed after 1 hr 13 min

@jenkins-droid
Copy link
Collaborator

Jenkins Linux s390x Build #15724 [push] Handle the options passe... passed after 1 hr 41 min

@jenkins-droid
Copy link
Collaborator

Jenkins Linux ppc64le Build #14751 [push] Handle the options passe... passed after 2 hr 22 min

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