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(codegen): type safe inkwell types #202

Merged
merged 7 commits into from
Jun 2, 2020

Conversation

baszalmstra
Copy link
Collaborator

@baszalmstra baszalmstra commented May 23, 2020

Inkwell uses types that lose a lot of type information. An u8, u16, i8, and 16 are all represented by an IntValue. This takes some careful management of these types. For IntValue the problem is not so bad but for PointerValue and StructValue this is a real issue. By using StructValue you lose all information in the type with what kind of value you are actually working. This is not so much a problem for dynamic types (like Mun structs) but it is a major risk when using abi types. I feel that this is major technical depth.

This PR introduces a new type Value<T> that contains an inkwell value but it is typed on T which is the type that constructed the value. This allows you to write:

let a:Value<SomeStruct>;           // Holds an inkwell StructValue
let b:Value<*const SomeStruct>;    // Holds an inkwell PointerValue

It also allows much cleaner conversion from regular Rust types to inkwell types. e.g.

let a:BasicValueEnum = 3u32.as_value(context).into();
let b:ArrayValue = [0.0f32, 6.0f32].as_value(context).into();
let c:StructValue = (90, true).as_value(context).into();

There are 3 ways to enable custom types to work with this system:

  1. Implement the TransparentValue trait which converts the implementor into another Value<T>. But in turn it also allows the implementor to be represented as a Value<Self>:

    use mun_codegen::value::{AsValue, IrValueContext, TransparentValue, Value};
    struct Foo {
       value: u32,
       bar: f32,
    }
    
     impl TransparentValue for Foo {
        type Target = (u32, f32);   
    
         fn as_target_value(&self, c ontext: &IrValueContext) -> Value<Self::Target> {
            (self.value, self.bar).as_value(context)
        }
    }

    The resulting IR type will be an anonymous type: type { u32, f32 }

  2. Auto derive the AsValue trait e.g.:

    #[derive(AsValue)]
    #[ir_name = "Foo"]
    pub struct Foo {
        value: u32,
        bar: f32,
    }

    The resulting IR type will be a named type: %Foo = type { u32, f32 }

  3. You can also implement all the support traits yourself for a custom type.

I already found a few bugs in the LLVM ir that have been fixed.

TODO

  • Refactor abi_types.rs to use the types from Value<T>s
  • Refactor the symbol and type-table generation to use the safe new Value<T> types
  • Create a procedural macro to validate that a certain structs matches its abi counterpart. (ir::TypeInfo should equal abi::TypeInfo)
  • See if we can remove some of the previous implementations (in ir.rs) (do in a later PR)

@baszalmstra baszalmstra added pri: intermediate An issue resulting in non-critical functionality loss and no significant effect on usability exp: intermediate Achievable by experienced contributors, or with some guidance type: refactor Refactor existing code labels May 23, 2020
@baszalmstra baszalmstra added this to the Mun v0.3.0 milestone May 23, 2020
@baszalmstra baszalmstra self-assigned this May 23, 2020
@codecov
Copy link

codecov bot commented May 23, 2020

Codecov Report

Merging #202 into master will decrease coverage by 0.86%.
The diff coverage is 65.85%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #202      +/-   ##
==========================================
- Coverage   82.95%   82.08%   -0.87%     
==========================================
  Files         163      174      +11     
  Lines       11514    11665     +151     
==========================================
+ Hits         9551     9575      +24     
- Misses       1963     2090     +127     
Impacted Files Coverage Δ
crates/mun_codegen/src/ir/function.rs 95.83% <ø> (ø)
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_codegen/src/value/string.rs 21.42% <21.42%> (ø)
crates/mun_codegen/src/ir.rs 47.50% <28.57%> (+4.31%) ⬆️
crates/mun_codegen/src/value/int_value.rs 46.42% <46.42%> (ø)
crates/mun_codegen/src/value/mod.rs 54.83% <54.83%> (ø)
crates/mun_codegen/src/value/global.rs 70.58% <70.58%> (ø)
crates/mun_codegen/src/value/tuple_value.rs 71.42% <71.42%> (ø)
crates/mun_codegen/src/code_gen.rs 89.41% <75.00%> (-4.40%) ⬇️
... and 26 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 3f67612...305b008. Read the comment docs.

@baszalmstra baszalmstra force-pushed the feature/inkwell_types branch from 61c12b2 to f6d8cb3 Compare May 25, 2020 20:18
@baszalmstra baszalmstra force-pushed the feature/inkwell_types branch from 9352194 to a4ed234 Compare May 27, 2020 22:37
@baszalmstra baszalmstra marked this pull request as ready for review May 31, 2020 10:49
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.

Props for this change. 👏 I had some minor requests for change.

crates/mun_codegen/Cargo.toml Outdated Show resolved Hide resolved
crates/mun_codegen/src/ir/type_table.rs Outdated Show resolved Hide resolved
crates/mun_codegen/src/ir/type_table.rs Outdated Show resolved Hide resolved
crates/mun_codegen/src/ir/type_table.rs Outdated Show resolved Hide resolved
crates/mun_codegen/src/ir/type_table.rs Outdated Show resolved Hide resolved
crates/mun_codegen/src/ir/types/test.rs Show resolved Hide resolved
crates/mun_codegen/src/value/global.rs Outdated Show resolved Hide resolved
crates/mun_codegen/src/value/global.rs Outdated Show resolved Hide resolved
crates/mun_codegen/src/value/global.rs Outdated Show resolved Hide resolved
crates/mun_codegen/src/value/array_value.rs Show resolved Hide resolved
@baszalmstra baszalmstra force-pushed the feature/inkwell_types branch from 0b64c3b to 305b008 Compare June 2, 2020 12:09
@baszalmstra baszalmstra changed the title WIP: Type safe inkwell types refactor(codegen): type safe inkwell types Jun 2, 2020
@baszalmstra baszalmstra merged commit 9f4c8d7 into mun-lang:master Jun 2, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
exp: intermediate Achievable by experienced contributors, or with some guidance pri: intermediate An issue resulting in non-critical functionality loss and no significant effect on usability type: refactor Refactor existing code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants