Skip to content

Add options to modify llvm-kompile's parameters in the Kompile frontend#2340

Merged
rv-jenkins merged 9 commits intomasterfrom
kompile-llvm-library
Dec 1, 2021
Merged

Add options to modify llvm-kompile's parameters in the Kompile frontend#2340
rv-jenkins merged 9 commits intomasterfrom
kompile-llvm-library

Conversation

@gtrepta
Copy link
Copy Markdown
Contributor

@gtrepta gtrepta commented Nov 22, 2021

fixes: #2088

Usage: kompile --backend llvm ... --llvm-kompile-type <main|search|library|static> --llvm-kompile-output <filename> ...

This gives better access to llvm-kompile's parameters instead of going through --no-llvm-kompile.

@gtrepta gtrepta marked this pull request as ready for review November 22, 2021 22:10
Copy link
Copy Markdown
Contributor

@dwightguth dwightguth left a comment

Choose a reason for hiding this comment

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

This needs a bit of work.

@dwightguth dwightguth dismissed their stale review November 22, 2021 22:25

pressed enter too soon

Copy link
Copy Markdown
Contributor

@dwightguth dwightguth left a comment

Choose a reason for hiding this comment

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

This needs a bit of work:

  1. First, we should call the flag something like llvm-kompile-output-type and it should take an enumeration with the following values: main, search, static, library.
  2. You should call the second flag llvm-kompile-output and it should probably not have a short option version. It should override the default output path in all cases and it should be an error to use together with --enable-search.
  3. We need better documentation of exactly what these flags do and what they are used for. That includes describing the intended use case of each of the 4 enumeration values I named above, and more detailed descriptions of both flags that don't assume knowledge of the internals of the backend.

In case you're unaware, here's a bit about each of the 4 values I mentioned above:

  1. main. Builds an interpreter and links in the main method. Generates an executable unless other flags are explicitly passed to llvm-kompile
  2. search. Same as main, except the executable does search instead of single-path execution.
  3. library. Same as main, except that it does not link in a main method. The user is expected to use it either by explicitly linking in their own main method.
  4. static. Same as library, except it does not pass any -l flags to clang when doing linking. This is useful primarily if you are passing -r -nostdlib to llvm-kompile in order to perform partial linking and generate an object file that can be linked later on a machine that does not have K installed.

@gtrepta
Copy link
Copy Markdown
Contributor Author

gtrepta commented Nov 24, 2021

@dwightguth

  • What should the behavior be with --llvm-kompile-output-type search? Should that set the --enable-search flag?
  • That would also mean --llvm-kompile-output-type search --llvm-kompile-output xxxx should throw an error?
  • Are you expecting the documentation to be in the --help output, or somewhere else like the user manual?

@dwightguth
Copy link
Copy Markdown
Contributor

  1. No, it shouldn't set --enable-search, it should just pass search as the type instead of main or library.
  2. No, it should be fine for the user to pass whatever output path they want when they select search.
  3. A short description should at the very least be in the --help message. We can put more elaborate details in the user manual if the description in the help message isn't complete. I just would prefer the help message be something that focuses on the use case rather than a simple set of factual technical details that are opaque to users not familiar with them.

@gtrepta
Copy link
Copy Markdown
Contributor Author

gtrepta commented Nov 24, 2021

@dwightguth re-requested your review.

I want to mention that library and static will cause clang to error out if the appropriate additional arguments are not passed in. This isn't mentioned so far in the --help text since it's already kind of long and hard to read. But I'm wondering if this behavior is fine with you?

@gtrepta gtrepta requested a review from dwightguth November 24, 2021 22:39
@gtrepta gtrepta changed the title Add options to use llvm-kompile ... library ... to Kompile frontend Add options to modify llvm-kompile's parameters in the Kompile frontend Nov 24, 2021
Copy link
Copy Markdown
Contributor

@dwightguth dwightguth left a comment

Choose a reason for hiding this comment

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

looks good but please update the help text as suggested before merging.

Comment thread llvm-backend/src/main/java/org/kframework/backend/llvm/LLVMKompileOptions.java Outdated
Comment thread llvm-backend/src/main/java/org/kframework/backend/llvm/LLVMKompileOptions.java Outdated
@rv-jenkins rv-jenkins merged commit 10adfd9 into master Dec 1, 2021
@rv-jenkins rv-jenkins deleted the kompile-llvm-library branch December 1, 2021 20:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Allow calling llvm-kompile in library mode through kompile frontend

3 participants