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.

Sign up for GitHub

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

Jump to bottom

Support llvm-sys 150.x.x #383

Merged
merged 37 commits into from
Jan 18, 2023
Merged

Support llvm-sys 150.x.x #383

merged 37 commits into from
Jan 18, 2023

Conversation

hedgar2017
Copy link
Contributor

@hedgar2017 hedgar2017 commented Nov 17, 2022 •
edited
Loading

Updates

  1. Support the opaque pointers and all related functions.
  2. Guard the LLVMConst* function family.
  3. Guard some optimization passes, including those for coroutines.
  4. Update the AtomicRMWBinOp enum.
  5. Add conditional compilation clauses for some low-hanging functions.
  6. Fix the AddressSpace enum and make it more generic (breaking).
  7. Add functions for parsing LLVM IR from the memory and files.
  8. Add the function for parsing command line options.
  9. The bitcast operation now returns AnyType to support function pointer casts.
  10. Some minor fixes and improvements.
  11. Updated some dependencies.

Notes

  1. I decided not to continue nesting in the functions which require opaque pointers, but instead implement another set of function with the suffix _2. It's not a perfect solution, but the alternative one would be worse from my point of view.
  2. The PR wasn't yet sanitized with clippy, and some tests are probable missing. Let's discuss. Honestly I feel like the project hasn't been sanitized in multiple previous PRs as well.

Related issues

#359

Supersedes this PR

ksolana and others added 7 commits October 31, 2022 17:12
@ksolana
Support llvm-15 for inkwell
3a97796
@ksolana
Add llvm15-0 to supported features
fffe576
Add the zkSync VM local changes
9dacd28
Add the LLVMParseCommandLineOptions binding
09c83e7
Change the dependencies protocol to HTTPS
30efb42
Make 'inkwell' compiler with LLVM 15
2a5b193 
1. Remove the `LLVMConst*` function family
2. Remove some optimization passes, including those for coroutines
3. Update the `AtomicRMWBinOp` enum
Make 'inkwell' compile with LLVM 15
47bd6a0 
1. Remove the `LLVMConst*` function family
2. Remove some optimization passes, including those for coroutines
3. Update the `AtomicRMWBinOp` enum

(cherry picked from commit 2a5b193)
@hedgar2017 hedgar2017 marked this pull request as draft November 17, 2022 21:14
@hedgar2017 hedgar2017 force-pushed the az/update-llvm-15-support branch from 4751e93 to 0bd11e5 Compare November 24, 2022 13:03
@hedgar2017 hedgar2017 marked this pull request as ready for review November 24, 2022 13:06
@hedgar2017 hedgar2017 mentioned this pull request Nov 24, 2022
Closed
Remove a function type check
5fedbd3 
(cherry picked from commit 124a088)
@TheDan64
Copy link
Owner

Hmm it seems like LLVM 15 isn't yet supported in the actions build? KyleMayes/install-llvm-action#31

@TheDan64 TheDan64 self-requested a review November 27, 2022 02:38
TheDan64
TheDan64 reviewed Nov 27, 2022
View reviewed changes
Cargo.toml Outdated Show resolved Hide resolved
src/builder.rs Outdated Show resolved Hide resolved
src/builder.rs Outdated Show resolved Hide resolved
src/builder.rs Outdated Show resolved Hide resolved
src/builder.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
src/passes.rs Show resolved Hide resolved
src/support/mod.rs Outdated Show resolved Hide resolved
src/values/float_value.rs Outdated Show resolved Hide resolved
@hedgar2017 hedgar2017 requested a review from TheDan64 November 27, 2022 09:18
TheDan64
TheDan64 approved these changes Dec 2, 2022
View reviewed changes
Copy link
Owner

@TheDan64 TheDan64 left a comment

Choose a reason for hiding this comment

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

Left a couple small comments/questions but otherwise approved. Would also like to see CI running tests for LLVM 15 but I'm not sure what we can do about the issue..

src/basic_block.rs Show resolved Hide resolved
tests/all/test_passes.rs Show resolved Hide resolved
tests/all/test_values.rs Outdated Show resolved Hide resolved
@hedgar2017 hedgar2017 requested review from TheDan64 and folkertdev and removed request for folkertdev and TheDan64 December 21, 2022 10:30
@folkertdev
Copy link
Contributor

so if we want to take this in the opaque pointer direction, one thing that still needs to happen is removing get_element_type on PointerType

@TheDan64
Copy link
Owner

I thought we agreed to move opaque support into a separate PR?

@folkertdev
Copy link
Contributor

well in that case many changes here (e.g. passing the extra arguments to load/GEP) are not required. We have now solved most of the technical issues so the only reason for that split really is that this PR is kind of big. On the other hand looking at the changes, most of them aren't very interesting. Really only the builder is interesting because type signatures change there and that means some APIs now have to be different.

anyway, up to @hedgar2017 I guess. I can report that this branch already works very well for my purposes.

@hedgar2017
Copy link
Contributor Author

@folkertdev this branch works for me perfectly as well.
On the contrary, I'll probably need to adjust my FE for the changes you propose, so I fully support the idea to proceed in another PR.
For now I use my custom branch which I'm going to rebase onto this one when this PR is merged.

@TheDan64 give me 1 sec, I'll fix those values I've missed for some reason.

@hedgar2017
Copy link
Contributor Author

@TheDan64 please re-run the CI.

@TheDan64
Copy link
Owner

Is this ready for final review, then?

@hedgar2017
Copy link
Contributor Author

Is this ready for final review, then?

Yes. Requested.

@baszalmstra baszalmstra mentioned this pull request Jan 4, 2023
Closed
TheDan64
TheDan64 reviewed Jan 7, 2023
View reviewed changes
Copy link
Owner

@TheDan64 TheDan64 left a comment

Choose a reason for hiding this comment

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

I'm giving this a soft approval - had a couple questions to address before merging.

Thanks for everyone who helped contribute!

examples/kaleidoscope/implementation_typed_pointers.rs Show resolved Hide resolved
src/builder.rs Show resolved Hide resolved
src/builder.rs Show resolved Hide resolved
src/builder.rs Outdated Show resolved Hide resolved
tests/all/test_instruction_values.rs Show resolved Hide resolved
tests/all/test_instruction_values.rs Outdated Show resolved Hide resolved
tests/all/test_passes.rs Show resolved Hide resolved
src/types/ptr_type.rs Show resolved Hide resolved
src/types/ptr_type.rs Show resolved Hide resolved
@hedgar2017
Copy link
Contributor Author

@TheDan64 I've landed some fixes. Please re-run the CI!

@hedgar2017
Copy link
Contributor Author

@TheDan64 fixed, please re-run.

@hedgar2017
Copy link
Contributor Author

@TheDan64 fixed, please re-run :)
(sorry, for some reason doc tests didn't run for me locally)

@TheDan64 TheDan64 merged commit 55770a8 into TheDan64:master Jan 18, 2023
@TheDan64 TheDan64 mentioned this pull request Jan 18, 2023
Closed
@Pat-Lafon Pat-Lafon mentioned this pull request Jan 18, 2023
Merged
1 task
@hedgar2017 hedgar2017 deleted the az/update-llvm-15-support branch January 18, 2023 06:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@folkertdev folkertdev folkertdev left review comments

@TheDan64 TheDan64 TheDan64 approved these changes

Assignees

@hedgar2017 hedgar2017

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@hedgar2017 @TheDan64 @ksolana @folkertdev @infality