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

refactor: upgrade to official inkwell #254

Merged
merged 1 commit into from
Aug 23, 2020

Conversation

baszalmstra
Copy link
Collaborator

@baszalmstra baszalmstra commented Aug 22, 2020

This PR eliminates the need to maintain a custom version of Inkwell. We used a custom version of Inkwell because our code could not handle a breaking change in their code; the lifetime of the inkwell::context::Context. Adding this lifetime was hard because we cached a lot of Inkwell related computations in the salsa IrDatabase. However, salsa doesn't work with lifetimes, so we couldn't add the lifetimes. Also, the LLVM context is single threaded which caused our database to not be Sync and we couldn't perform parallel LLVM computations.

This PR adds the context lifetime throughout the codebase enabling us to use the latest (and officially) released version of Inkwell. This will also allow us to finally publish the compiler crates.

Besides the inkwell update, the code is also updated to work with LLVM8 (over LLVM7). In the future we should be able to update to LLVM10+, however, due to a [bug in LLD introduced in LLVM 9][https://reviews.llvm.org/D86401) we couldn't invoke the linker multiple times. Once the fix for that bug lands in a release version of LLVM we can upgrade further (inkwell already supports LLVM10).

Related to inkwell issues: TheDan64/inkwell#187

Closes #151

@codecov
Copy link

codecov bot commented Aug 22, 2020

Codecov Report

Merging #254 into master will decrease coverage by 0.14%.
The diff coverage is 82.64%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #254      +/-   ##
==========================================
- Coverage   78.63%   78.49%   -0.15%     
==========================================
  Files         212      215       +3     
  Lines       12681    12719      +38     
==========================================
+ Hits         9972     9984      +12     
- Misses       2709     2735      +26     
Impacted Files Coverage Δ
crates/mun_codegen/src/code_gen.rs 100.00% <ø> (+12.50%) ⬆️
crates/mun_codegen/src/ir/types/test.rs 0.00% <0.00%> (ø)
crates/mun_codegen/src/linker.rs 22.35% <ø> (ø)
crates/mun_codegen/src/value/float_value.rs 0.00% <0.00%> (ø)
crates/mun_codegen_macros/src/lib.rs 0.00% <0.00%> (ø)
crates/mun_compiler/src/driver.rs 59.64% <ø> (ø)
crates/mun_runtime/tests/runtime.rs 100.00% <ø> (ø)
crates/mun_target/src/spec.rs 80.00% <ø> (ø)
crates/mun_target/src/spec/windows_msvc_base.rs 100.00% <ø> (ø)
crates/mun_codegen/src/value/string.rs 21.42% <25.00%> (ø)
... and 33 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7bc27ba...97f4ca8. Read the comment docs.

@baszalmstra baszalmstra added this to the Mun v0.3.0 milestone Aug 22, 2020
@baszalmstra baszalmstra self-assigned this Aug 22, 2020
@baszalmstra baszalmstra added exp: high Achievable by investing significant time and effort, potentially through collaboration pri: high An issue resulting in complete or substantial loss of functionality, that can be circumvented type: refactor Refactor existing code and removed exp: high Achievable by investing significant time and effort, potentially through collaboration pri: high An issue resulting in complete or substantial loss of functionality, that can be circumvented labels Aug 22, 2020
@baszalmstra baszalmstra changed the title WIP: refactor: upgrade to official inkwell refactor: upgrade to official inkwell Aug 23, 2020
Copy link
Collaborator

@Wodann Wodann left a comment

Choose a reason for hiding this comment

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

Finally, we'll be able to publish all of our crates. Nice! 🎉

@baszalmstra baszalmstra force-pushed the feat/llvm_upgrade branch 2 times, most recently from 09bb921 to bd9df1c Compare August 23, 2020 12:09
@baszalmstra baszalmstra merged commit 7147826 into mun-lang:master Aug 23, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: refactor Refactor existing code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update to official inkwell crate
2 participants