Skip to content

fix(code_gen): incremental compilation#101

Merged
baszalmstra merged 3 commits intomun-lang:masterfrom
Wodann:fix/incremental-compilation
Mar 23, 2020
Merged

fix(code_gen): incremental compilation#101
baszalmstra merged 3 commits intomun-lang:masterfrom
Wodann:fix/incremental-compilation

Conversation

@Wodann
Copy link
Collaborator

@Wodann Wodann commented Mar 22, 2020

Resolves all three side-effects of PR #97.

@Wodann Wodann requested a review from baszalmstra March 22, 2020 15:45
@Wodann Wodann self-assigned this Mar 22, 2020
This prevents usage of out of scope IR, e.g. due to copying of an LLVM
module
@Wodann Wodann force-pushed the fix/incremental-compilation branch from 39f396a to ce7d9e0 Compare March 22, 2020 15:53
@codecov
Copy link

codecov bot commented Mar 22, 2020

Codecov Report

Merging #101 into master will decrease coverage by 7.60%.
The diff coverage is 80.30%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #101      +/-   ##
==========================================
- Coverage   76.84%   69.24%   -7.61%     
==========================================
  Files         132      133       +1     
  Lines       10643    10763     +120     
==========================================
- Hits         8179     7453     -726     
- Misses       2464     3310     +846     
Impacted Files Coverage Δ
crates/mun_codegen/src/code_gen.rs 61.83% <0.00%> (-26.79%) ⬇️
crates/mun_codegen/src/ir.rs 55.76% <ø> (ø)
crates/mun_runtime/src/test.rs 19.09% <0.00%> (-76.99%) ⬇️
crates/mun_codegen/src/ir/abi_types.rs 86.58% <50.00%> (-13.42%) ⬇️
crates/mun_codegen/src/code_gen/symbols.rs 37.85% <62.96%> (-56.76%) ⬇️
crates/mun_codegen/src/ir/ty.rs 93.00% <75.00%> (-1.00%) ⬇️
crates/mun_codegen/src/ir/file.rs 79.31% <79.31%> (ø)
crates/mun_codegen/src/ir/body.rs 85.38% <86.20%> (-5.96%) ⬇️
crates/mun_codegen/src/ir/dispatch_table.rs 89.92% <93.33%> (-0.08%) ⬇️
crates/mun_codegen/src/ir/type_table.rs 93.71% <95.23%> (-3.91%) ⬇️
... and 38 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 0400406...8d5f1d5. Read the comment docs.

Copy link
Collaborator

@baszalmstra baszalmstra left a comment

Choose a reason for hiding this comment

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

Very nice! This also cleans up the generated IR by a lot.

  • IR of the group_ir currently doesnt have a regression test
  • Im missing documentation on what the group IR is.

Side note: did you try if incremental compilation works?

@Wodann
Copy link
Collaborator Author

Wodann commented Mar 23, 2020

  • IR of the group_ir currently doesnt have a regression test

What do you think is the best solution:

  • Output regression tests for the group_ir and module_ir?
  • Output a single regression test for the linked (and potentially optimized) IR

Side note: did you try if incremental compilation works?

I don't currently have a solution for testing this. Do you have any ideas on how we could do it? This PR merely restores things to the status quo before PR #97.

@Wodann Wodann force-pushed the fix/incremental-compilation branch from ce7d9e0 to ca950ac Compare March 23, 2020 10:02
@Wodann
Copy link
Collaborator Author

Wodann commented Mar 23, 2020

I've force pushed the requested additional comments. I also changed some of the file and struct names to better self-document, most notably module::ModuleIR -> file::FileIR. I felt this better reflects the changed structure.

I also changed the file::FileIR's return type to no longer contain FunctionValues, since those are only valid within the original LLVM Module, so it is unsafe to pass them around.

To facilitate this change, I also modified this and that.

@baszalmstra
Copy link
Collaborator

What do you think is the best solution:

* Output regression tests for the `group_ir` **and** `module_ir`?

* Output a single regression test for the linked (and potentially optimized) IR

Good question, I think both? But if I'd have to choose one, then probably the first.

I don't currently have a solution for testing this. Do you have any ideas on how we could do it? This PR merely restores things to the status quo before PR #97.

There is already one such test in https://github.com/mun-lang/mun/blob/master/crates/mun_hir/src/tests.rs . But I agree, maybe put that in a new PR. I do feel that it becomes more and more important as we go further. We want to make sure that we do not break incremental compilation. As we have seen that's very easy to do.

@Wodann Wodann force-pushed the fix/incremental-compilation branch from ca950ac to 8d5f1d5 Compare March 23, 2020 10:51
@baszalmstra baszalmstra merged commit 00cb85a into mun-lang:master Mar 23, 2020
@Wodann Wodann deleted the fix/incremental-compilation branch March 25, 2020 11:26
@Wodann Wodann added this to the Mun v0.2 milestone May 14, 2020
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