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

feat: object ptr indirection #97

Merged
merged 7 commits into from
Mar 21, 2020

Conversation

Wodann
Copy link
Collaborator

@Wodann Wodann commented Mar 13, 2020

Beware of several changes:

  1. target triple included in snapshots
  2. type info included in snapshots (as a result, target triples are also needed)
  3. inclusion of LLVM module in #[salsa::input] which might affect incremental compilation.

[IMPORTANT] To unblock PR #98, a resolution for 3) will be provided in a future PR!

@Wodann Wodann requested a review from baszalmstra March 13, 2020 12:36
@Wodann Wodann self-assigned this Mar 13, 2020
@codecov
Copy link

codecov bot commented Mar 13, 2020

Codecov Report

Merging #97 into master will increase coverage by 0.27%.
The diff coverage is 94.12%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #97      +/-   ##
==========================================
+ Coverage   76.57%   76.84%   +0.27%     
==========================================
  Files         127      130       +3     
  Lines       10275    10569     +294     
==========================================
+ Hits         7868     8122     +254     
- Misses       2407     2447      +40     
Impacted Files Coverage Δ
crates/mun_codegen/src/code_gen/linker.rs 35.22% <0.00%> (-0.41%) ⬇️
crates/mun_codegen/src/ir/function.rs 92.00% <0.00%> (ø)
crates/mun_codegen/src/lib.rs 100.00% <ø> (ø)
crates/mun_compiler/src/driver/config.rs 63.63% <ø> (ø)
crates/mun_compiler/src/lib.rs 0.00% <0.00%> (ø)
crates/mun_target/src/spec/x86_64_apple_darwin.rs 0.00% <0.00%> (ø)
crates/mun_runtime/src/reflection.rs 18.98% <33.33%> (-2.45%) ⬇️
crates/mun_hir/src/code_model.rs 75.78% <57.14%> (-0.62%) ⬇️
crates/mun_codegen/src/type_info.rs 56.52% <60.00%> (-8.48%) ⬇️
crates/mun_abi/src/autogen.rs 57.22% <66.66%> (+4.94%) ⬆️
... and 47 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 decbf12...325b66a. 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.

We have to find a better solution for storing LLVM stuff in salsa. Otherwise, very nice :)

crates/mun_abi/src/autogen_impl.rs Show resolved Hide resolved
crates/mun_codegen/src/code_gen.rs Show resolved Hide resolved
crates/mun_codegen/src/db.rs Show resolved Hide resolved
crates/mun_codegen/src/db.rs Show resolved Hide resolved
crates/mun_codegen/src/intrinsics.rs Show resolved Hide resolved
@@ -104,6 +119,19 @@ pub(crate) fn ir_query(db: &impl IrDatabase, file_id: FileId) -> Arc<ModuleIR> {
let dispatch_table = dispatch_table_builder.finalize(&functions);
let fn_pass_manager = function::create_pass_manager(&llvm_module, db.optimization_lvl());

// Create the allocator handle global value
let allocator_handle_global = if dispatch_table.has_intrinsic(&crate::intrinsics::new) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

the allocato_handle_global should also be present if the clone intrinsic is present.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

If new is there, clone must also be there - and vice versa.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Nonetheless if that is not the case in the future this will silently fail. Best to add it just in case.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The new, clone, and - in the future - drop are all object management functions. They are all required if a struct type is defined. That should be the real test, I case because checking for everything potential future object management function doesn't scale too well.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I agree but this check is to determine whether or not a global variable allocator handle is required. This is only the case if either of these methods is required no matter where they are used. Maybe in the future we would like to use one of these functions for a different use case. Than we dont have to touch the check.

The best check would be to check if one of the functions in the dispatch table are actually using the AllocatorHandle.

crates/mun_codegen/src/ir/type_table.rs Show resolved Hide resolved
crates/mun_codegen/src/ir/type_table.rs Show resolved Hide resolved
}
hir::StructMemoryKind::Value => {
if self.params.make_marshallable {
self.builder.build_load(value.into_pointer_value(), "deref")
let mem_ptr = self
Copy link
Collaborator

Choose a reason for hiding this comment

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

Duplicate code

@@ -469,11 +496,21 @@ impl<'a, 'b, D: IrDatabase> BodyIrGenerator<'a, 'b, D> {
..
}) => match s.data(self.db).memory_kind {
hir::StructMemoryKind::GC => {
self.builder.build_load(value.into_pointer_value(), "deref")
let mem_ptr = self
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe explain what is going on here?

@baszalmstra
Copy link
Collaborator

So while looking a bit further, I found: http://llvm.org/doxygen/CloneModule_8cpp_source.html

This clearly shows what gets copied and what doesn't. I already found that you can re-lookup global variables and function definitions by name. Maybe we can write a little wrapper around modules, global values and function definitions that enables us to better clone these things?

@Wodann
Copy link
Collaborator Author

Wodann commented Mar 16, 2020

I don't think this will help the situation. The generation of module IR requires target data information. You mentioned that that is not allowed to be set in the salsa db, so we need to move module IR generation out of salsa. Thus there is no need for us to copy anything. We can see if we can move more modular parts to salsa, though?

@baszalmstra
Copy link
Collaborator

I don't think this will help the situation. The generation of module IR requires target data information. You mentioned that that is not allowed to be set in the salsa db, so we need to move module IR generation out of salsa. Thus there is no need for us to copy anything. We can see if we can move more modular parts to salsa, though?

We can totally store the target in the database! Its not a function of other input right?

@Wodann Wodann force-pushed the feature/object-ptr-indirection branch from e68435e to 325b66a Compare March 21, 2020 13:40
@baszalmstra baszalmstra merged commit dd8111c into mun-lang:master Mar 21, 2020
@Wodann Wodann deleted the feature/object-ptr-indirection branch March 21, 2020 17:50
@Wodann
Copy link
Collaborator Author

Wodann commented Mar 27, 2020

@Wodann Wodann changed the title Feature/object ptr indirection feat: object ptr indirection Apr 1, 2020
@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