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

LLVM command line options integrations #153

Merged
merged 4 commits into from
Oct 8, 2019

Conversation

mshockwave
Copy link
Contributor

@mshockwave mshockwave commented Aug 3, 2019

The proposed feature allows users of ponyc to supply LLVM-specific command line options from ponyc's command line interface in dev builds (i.e. Debug build). This should make development of ponyc easier by improving accessibilities of internal LLVM features during debugging and fine-grained tuning.


from the Pony core team:

Everyone who has an opinion on Option 1 vs Option 2, please leave a reaction to this comment where you do:

hooray reaction if you favor option 1 and rocket reaction if you favor option 2.

@SeanTAllen SeanTAllen requested review from jemc and removed request for jemc August 3, 2019 18:09
@dipinhora
Copy link

Julia's solution seems like a good alternative: https://docs.julialang.org/en/v1/manual/environment-variables/index.html#JULIA_LLVM_ARGS-1

@mshockwave
Copy link
Contributor Author

@dipinhora thanks for the feedback, I'll put into the alternative section.

And some rewording.
@SeanTAllen
Copy link
Member

Thanks for this @mshockwave.

We are going to discuss this on August 20th after trying to collect some feedback on the two options.

@dipinhora
Copy link

I'd prefer option 3: either an environment variable or a command line argument passed in using quotes ("to handle spaces") so that the entire blob can be passed off to LLVM directly without any pre-processing or thought about how to structure the arguments (to prefix them all with -mllvm or which separator to use instead of ).

From what I can tell, the Julia environment variable implementation boils down to 1 line of code (https://github.com/JuliaLang/julia/blob/master/src/codegen.cpp#L7613) calling into an existing LLVM function (https://llvm.org/doxygen/namespacellvm_1_1cl.html#ae5a3364c95fc71013039ef5fd7aa7fe0). I'm guessing there's a similar function for parsing all LLVM arguments from a single string also.

@jemc
Copy link
Member

jemc commented Aug 13, 2019

I like Dipin's suggestion.

@mshockwave
Copy link
Contributor Author

Actually I see no problem implementing all of the options, including option 3 proposed by @dipinhora . Option 1 and option 2 can exist at the same time. If user also supply environment variable in option 3, we just concat it before or after those which are supplied from command line.

@SeanTAllen
Copy link
Member

I'm not in favor of multiple ways to do this. I want 1 way to do it. Definitely not 2 or 3 possible ways.

@slfritchie
Copy link

Looking at the https://docs.julialang.org/en/v1/manual/environment-variables/index.html#JULIA_LLVM_ARGS-1 link, I don't see anything there that tells me what the contents of the JULIA_LLVM_ARGS env var is supposed to contain. Is it verbatim LLVM compiler args? Or something else. Some clarification and an example would be really helpful.

@mshockwave
Copy link
Contributor Author

mshockwave commented Aug 21, 2019

@slfritchie I think they simply use this API. Here is an usage example:

export MY_PONY_LLVM_ARGS="-debug-only=mem2reg,sroa -stats"
./ponyc <pony args>

somewhere in ponyc, we call the API like this:

llvm::cl::ParseEnvironmentOptions("ponyc", "MY_PONY_LLVM_ARGS");

In short, we put exactly the same CLI option string as we passed to other LLVM tools into the environment variable, and we can choose our own environment variable name.
However, as mentioned in the LLVM command line doc above: the API does not support quoting. That is, -some-args="foo bar" will be seen as two separated options -some-args=foo and bar with this API.

@SeanTAllen
Copy link
Member

Ok, we are down to vote time. 3 possibilities. Please vote as a reaction to this comment:

  • Command line option 2 as listed in the RFC add a 🚀
  • Environment variable listed in comments here and in alternatives ❤️
  • Allow for both of these 👀

@aturley
Copy link
Member

aturley commented Sep 3, 2019

The voting on this will close on September 10.

@SeanTAllen
Copy link
Member

This has been accepted in principal. @mshockwave can you update the RFC to make option 2 as "the option" and have the others as alternatives that we didn't accept, but did exist?

Once that is done, we can merge and officially accept.

Thanks.

- Mark option 2 as "the option"
- Put all the other options into the alternative section
@mshockwave
Copy link
Contributor Author

@SeanTAllen done.
Also just curious, after this RFC is accepted, are we going to proceed with the implementation?

@SeanTAllen
Copy link
Member

@mshockwave when accepted, an issue will be created on the ponyc that anyone can pick up and start working on. if you want to do the work, please do.

@SeanTAllen
Copy link
Member

This RFC has been accepted.

@SeanTAllen
Copy link
Member

@mshockwave there's an issue open for this now: ponylang/ponyc#3327

@mshockwave
Copy link
Contributor Author

@SeanTAllen awesome!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status - ready for vote The RFC is ready to be voted on.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants