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

Incremental codegen refactor to support llvm/inkwell upgrade #201

Conversation

joshuawarner32
Copy link

This is a WIP refactor of the codegen, with the end goal of allowing upgrading llvm to 10 and inkwell to the upstream version (#151). Critically in the upstream version of inkwell many of the types grow a <'ctx> type parameter (For the lifetime of the inkwell::context::Context object), which is not compatible with how we are using Salsa in mun_codegen.

To that end, this is a refactoring that removes the use of Salsa from the mun_codegen. IrDatabaseStorage is replaced with four separate items:

  • context: &Context (the inkwell context)
  • type_manager: &mut TypeManager (caches struct/type info like salsa used to)
  • config: &CodeGenConfig (holding the opt level and TargetData)
  • db: &impl hir::HirDatabase (the salsa context from mun_hir)

This massively increases the length of many method signatures, which I'm not a huge fan of - but I suspect some of these will need careful lifetime annotations, which might be harder to get right if we bundle everything together prematurely. For sure context will need to be passed around separately.

(Note, the first two commits here are 100% temporary; I need to rebase these away before this is merged)

@codecov
Copy link

codecov bot commented May 23, 2020

Codecov Report

Merging #201 into master will decrease coverage by 0.32%.
The diff coverage is 96.59%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #201      +/-   ##
==========================================
- Coverage   82.95%   82.63%   -0.33%     
==========================================
  Files         163      162       -1     
  Lines       11514    11503      -11     
==========================================
- Hits         9551     9505      -46     
- Misses       1963     1998      +35     
Impacted Files Coverage Δ
crates/mun_codegen/src/mock.rs 89.47% <ø> (-7.50%) ⬇️
crates/mun_compiler/src/db.rs 75.00% <ø> (ø)
crates/tools/src/lib.rs 74.07% <ø> (+1.34%) ⬆️
crates/mun_codegen/src/code_gen.rs 91.59% <80.00%> (-2.21%) ⬇️
crates/mun_codegen/src/ir/body.rs 84.94% <94.44%> (-0.12%) ⬇️
crates/mun_codegen/src/ir/ty.rs 95.69% <95.34%> (-4.31%) ⬇️
crates/mun_codegen/src/code_gen/symbols.rs 94.00% <100.00%> (-0.08%) ⬇️
crates/mun_codegen/src/ir/adt.rs 100.00% <100.00%> (ø)
crates/mun_codegen/src/ir/dispatch_table.rs 85.47% <100.00%> (+0.85%) ⬆️
crates/mun_codegen/src/ir/file.rs 100.00% <100.00%> (ø)
... and 14 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 0011016...7419695. Read the comment docs.

@joshuawarner32 joshuawarner32 force-pushed the incremental-codegen-refactor branch from 064ce8f to 7419695 Compare June 2, 2020 04:47
@baszalmstra
Copy link
Collaborator

Hey @joshuawarner32 !

After some further investigation, we spotted some issues with our current design in Mun regarding storing the inkwell Context in a salsa Database. To do parallelism we would need to create a Context per thread. This conflicts with the current design that a Context is stored in a salsa database that is shared between threads. We discussed how we're going to solve this and decided that for now we're just not gonna use Salsa for the code generation. This is basically what you were doing all along 😅 .

I just wanted to let you know that we're currently taking that route. I was looking at your PR to see if we could salvage some of what you already made, but your PR is way behind the current master branch.

I'll be working on getting this up and running in the next couple of days, if you would still like to help out, that'd be more than welcome!

@joshuawarner32
Copy link
Author

Oh interesting!

To do parallelism we would need to create a Context per thread.

My immediate reaction is that this could make the lifetimes very tricky, since all the different contexts would technically have different lifetimes. But maybe this is ok, depending on at what level you do the multithreading at. You probably can't have objects tied to one thread be accessed on another. (??)

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