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

Add new pass manager API #50

Merged
merged 3 commits into from
Sep 20, 2023
Merged

Add new pass manager API #50

merged 3 commits into from
Sep 20, 2023

Conversation

rj45
Copy link
Contributor

@rj45 rj45 commented Aug 25, 2023

Just in case you want this code, I implemented the new pass manager. Looks like soon this will be the only pass manager API available as they will be removing the old APIs.

This API looks like it's been available since LLVM 14 or earlier, so I think it should be safe.

I added a test.... it's mostly copy pasta from another test, but hey it's better than nothing, and it checks that the error handling code is correct.

Let me know if you're interested in these PRs. If you're not, I can just maintain my own fork. I understand you not wanting to maintain code that is not depended upon by tinygo. Though, in this case, I think you might want this code as it will be required eventually.

Feel free to close this PR and go a different direction, or make any edits you would like, I don't mind.

@aykevl
Copy link
Member

aykevl commented Aug 29, 2023

Let me know if you're interested in these PRs. If you're not, I can just maintain my own fork. I understand you not wanting to maintain code that is not depended upon by tinygo.

I am! I think it's much more useful to work together on a single repo than if everybody maintains their own fork.
And while it is currently primarly meant for TinyGo, that's mainly because there are very few other (active!) projects that require Go bindings to LLVM. This could certainly change.

I'll take a more in-depth look at a later time.

@rj45
Copy link
Contributor Author

rj45 commented Sep 2, 2023

Accidentally put the CI fix on the wrong branch. Let me know if I should instead force push without the commits.

Copy link
Member

@aykevl aykevl left a comment

Choose a reason for hiding this comment

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

Looks reasonable to me. I'll test this with TinyGo to see whether the API also works there.

One possible improvement is to add more documentation comments. Most functions in the go-llvm API don't have that, but that's no reason to not add them for new APIs (the go-llvm package API is generally not exactly the best API design, to put it mildly).

passes_test.go Outdated
InitializeNativeTarget()
InitializeNativeAsmPrinter()

mod := NewModule("fac_module")
Copy link
Member

Choose a reason for hiding this comment

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

Can you change this test to avoid the global context? See #54 for details.

@aykevl
Copy link
Member

aykevl commented Sep 19, 2023

Let me know if I should instead force push without the commits.

I generally prefer to have a clean git history, so if you can remove the last two commits that would be great.

@aykevl
Copy link
Member

aykevl commented Sep 19, 2023

Tested this API with TinyGo and it works well.

Can you do the following to make this PR mergeable?

  • Rebase on the master branch.
  • Remove the last two commits (optional, but preferred).
  • Update the tests to avoid the global context (Add new pass manager API #50 (comment))
  • Optional: improve the doc comments for the new methods.

@rj45
Copy link
Contributor Author

rj45 commented Sep 20, 2023

@aykevl Done. I had to do some investigation to figure out what some of the options actually do, one or two I had to kind of extrapolate what it might do based on what little scraps of comments I could find on the option. Like forgetting the scalar evolution -- I am guessing that could prevent loop unrolling in some cases, but I couldn't find anything that said that, so I just said "it could affect loop unrolling in some cases".

Let me know if I missed anything when converting the test not to use the global context. I just assumed you removed all of the global context functions, so if the test passed I got them all.

@aykevl
Copy link
Member

aykevl commented Sep 20, 2023

Looks good to me, thank you for the update!

Let me know if I missed anything when converting the test not to use the global context. I just assumed you removed all of the global context functions, so if the test passed I got them all.

I hope I found all of them! At least I tried to.

@aykevl aykevl merged commit 7733695 into tinygo-org:master Sep 20, 2023
6 checks passed
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