ndstrides: [1] Introduce models #509

Closed
lyken wants to merge 1 commits from ndstrides-1-model into ndstrides
Collaborator

Added the Model<'ctx> abstraction. This is a thin layer of Rust type-hints over LLVM values and types.

It also has mechanisms to help defining new LLVM struct types with very little boilerplate. LLVM 15 opaque pointers have also been accounted for. Shortly there will be a PR that refactors nac3core with Models.

If this is undesired, I can rewrite the entire ndstrides branch to use Proxy{Value,Type}.

When put in practice, I think a531d0127a (diff-d202bc814a4d1cf1e6c1a040946e598e69197246) might give some ideas of how it is used.

Added the `Model<'ctx>` abstraction. This is a thin layer of Rust type-hints over LLVM values and types. It also has mechanisms to help defining new LLVM struct types with very little boilerplate. LLVM 15 opaque pointers have also been accounted for. Shortly there will be a PR that refactors nac3core with Models. If this is undesired, I can rewrite the entire ndstrides branch to use `Proxy{Value,Type}`. When put in practice, I think https://git.m-labs.hk/M-Labs/nac3/commit/a531d0127ae9c846fce1bbdbe391df0c2282caa9#diff-d202bc814a4d1cf1e6c1a040946e598e69197246 might give some ideas of how it is used.
lyken added 2 commits 2024-08-27 10:48:56 +08:00
lyken requested review from derppening 2024-08-27 10:53:19 +08:00
derppening requested changes 2024-08-27 15:52:31 +08:00
derppening left a comment
Collaborator

In addition to the comments provided,

  • Are there Into implementations to convert Instance into BasicValue and Model into BasicType?
  • The Traversal classes for Struct is really confusing me. Why is it necessary? Why couldn't it be made as a Vec and then use iter to collect information regarding each field?
In addition to the comments provided, - Are there `Into` implementations to convert `Instance` into `BasicValue` and `Model` into `BasicType`? - The `Traversal` classes for `Struct` is really confusing me. Why is it necessary? Why couldn't it be made as a `Vec` and then use `iter` to collect information regarding each field?
@ -0,0 +11,4 @@
use super::*;
/// Trait for Rust structs identifying length values for [`Array`].
pub trait LenKind: fmt::Debug + Clone + Copy {
Collaborator

If this is only specific to Array (and not other kinds of containers), it might be better if it's named as ArrayLen.

If this is only specific to `Array` (and not other kinds of containers), it might be better if it's named as `ArrayLen`.
derppening marked this conversation as resolved
@ -0,0 +12,4 @@
/// Trait for Rust structs identifying length values for [`Array`].
pub trait LenKind: fmt::Debug + Clone + Copy {
fn get_length(&self) -> u32;
Collaborator

I think you can just name it as length().

I think you can just name it as `length()`.
derppening marked this conversation as resolved
@ -0,0 +39,4 @@
///
/// `Len` should be of a [`LenKind`] and `Item` should be a of [`Model`].
#[derive(Debug, Clone, Copy, Default)]
pub struct Array<Len, Item> {
Collaborator

Why are the type variables not directly bounded by LenKind and Model respectively then, rather than only bounding the impls?

Why are the type variables not directly bounded by `LenKind` and `Model` respectively then, rather than only bounding the `impl`s?
Author
Collaborator

Bounding say Item: Model<'ctx> requires Array itself to also have 'ctx, then a _PhantomData.

Bounding say `Item: Model<'ctx>` requires `Array` itself to also have `'ctx`, then a `_PhantomData`.
Collaborator

PhantomData might be worth it in this case, because the entire Array class is bounded by the aforementioned type bounds, not just the specific impls.

`PhantomData` might be worth it in this case, because the entire `Array` class is bounded by the aforementioned type bounds, not just the specific `impl`s.
Author
Collaborator

I am not sure if this is a good idea. I intuitively think it is less risky by keeping the generics as lax as possible even if it seems very appealing to constraint the struct, though I only first touched Rust 3 months ago.

I was trying to foresee if there will be any super wacky unfixable because-of-the-Rust-type-system-and-its-limitations issue that will crop up in the future because we are over-traiting, but honestly I can't tell.

Also a minor drawback is that 'ctx is now everywhere when you write Array<'ctx, X, Y>, and this will include Int, Float, Struct and Ptr.

Though it can prevent confusions such as "why my Array<Len<3>, Int32> is not a model?? Oh, I have to put Int<Int32>" which is quite nice.

Leaving everything as it is also does not seem to have any drawbacks in my naive opinion.

I am not sure if this is a good idea. I intuitively think it is less risky by keeping the generics as lax as possible even if it seems very appealing to constraint the struct, though I only first touched Rust 3 months ago. I was trying to foresee if there will be any super wacky unfixable because-of-the-Rust-type-system-and-its-limitations issue that will crop up in the future because we are over-traiting, but honestly I can't tell. Also a minor drawback is that `'ctx` is now everywhere when you write `Array<'ctx, X, Y>`, and this will include `Int`, `Float`, `Struct` and `Ptr`. Though it can prevent confusions such as "why my `Array<Len<3>, Int32>` is not a model?? Oh, I have to put `Int<Int32>`" which is quite nice. Leaving everything as it is also does not seem to have any drawbacks in my naive opinion.
Collaborator

I wouldn't be too concerned with using PhantomData here. I'd rather the type bounds of the struct be explicit so that users of this class do not misuse the fields.

In fact, we should also introduce a new function here and make len and item non-pub imo.

I wouldn't be too concerned with using `PhantomData` here. I'd rather the type bounds of the struct be explicit so that users of this class do not misuse the fields. In fact, we should also introduce a `new` function here and make `len` and `item` non-`pub` imo.
@ -0,0 +88,4 @@
ctx: &CodeGenContext<'ctx, '_>,
i: IntValue<'ctx>,
) -> Instance<'ctx, Ptr<Item>> {
let zero = ctx.ctx.i32_type().const_zero();
Collaborator

Pretty sure this can be i64/usize - LLVM docs:

When indexing into an array, pointer or vector, integers of any width are allowed, and they are not required to be constant. These integers are treated as signed values where relevant.

Change this for the other array stuff as well.

Pretty sure this can be `i64`/`usize` - [LLVM docs](https://releases.llvm.org/14.0.0/docs/LangRef.html#getelementptr-instruction): > When indexing into an array, pointer or vector, integers of any width are allowed, and they are not required to be constant. These integers are treated as signed values where relevant. Change this for the other array stuff as well.
Author
Collaborator

I am not sure what should be changed for line 88-94.

You mean let zero = <something else?>

I am not sure what should be changed for line 88-94. You mean `let zero = <something else?>`
Collaborator
let zero = ctx.ctx.i64_type().const_zero();
```rs let zero = ctx.ctx.i64_type().const_zero(); ```
@ -0,0 +95,4 @@
}
/// Like `gep` but `i` is a constant.
pub fn gep_const(&self, ctx: &CodeGenContext<'ctx, '_>, i: u64) -> Instance<'ctx, Ptr<Item>> {
Collaborator

i64 - See above.

`i64` - See above.
Author
Collaborator

I am confused why we should use i: i64 in gep_const.

Shouldn't it be the case that 0 <= i < self.model.0.len.length()? even if indexing with a negative index is technically possible.

I am confused why we should use `i: i64` in `gep_const`. Shouldn't it be the case that `0 <= i < self.model.0.len.length()`? even if indexing with a negative index is technically possible.
@ -0,0 +102,4 @@
self.model.0.len.get_length()
);
let i = ctx.ctx.i32_type().const_int(i, false);
Collaborator
https://git.m-labs.hk/M-Labs/nac3/pulls/509/files#issuecomment-11752
@ -0,0 +11,4 @@
pub struct ModelError(pub String);
impl ModelError {
// Append a context message to the error.
Collaborator

nit: ///

nit: `///`
derppening marked this conversation as resolved
@ -0,0 +68,4 @@
/// Return the [`BasicType`] of this model.
#[must_use]
fn get_type<G: CodeGenerator + ?Sized>(&self, generator: &G, ctx: &'ctx Context) -> Self::Type;
Collaborator

I would suggest naming it llvm_type. Use of get_ is discouraged in Rust AFAIK.

https://rust-lang.github.io/api-guidelines/naming.html#getter-names-follow-rust-convention-c-getter

I would suggest naming it `llvm_type`. Use of `get_` is discouraged in Rust AFAIK. https://rust-lang.github.io/api-guidelines/naming.html#getter-names-follow-rust-convention-c-getter
derppening marked this conversation as resolved
@ -0,0 +71,4 @@
fn get_type<G: CodeGenerator + ?Sized>(&self, generator: &G, ctx: &'ctx Context) -> Self::Type;
/// Get the number of bytes of the [`BasicType`] of this model.
fn sizeof<G: CodeGenerator + ?Sized>(
Collaborator

I would suggest following Inkwell conventions - size_of.

I would suggest following Inkwell conventions - `size_of`.
derppening marked this conversation as resolved
@ -0,0 +91,4 @@
///
/// Caller must make sure the type of `value` and the type of this `model` are equivalent.
#[must_use]
fn believe_value(&self, value: Self::Value) -> Instance<'ctx, Self> {
Collaborator

I suspect this might be a candidate for unsafe, as it is a requirement for the caller to enforce this contract.,

I suspect this might be a candidate for `unsafe`, as it is a requirement for the caller to enforce this contract.,
derppening marked this conversation as resolved
@ -0,0 +152,4 @@
len: IntValue<'ctx>,
name: Option<&'ctx str>,
) -> Result<Instance<'ctx, Ptr<Self>>, String> {
// TODO: Remove ArraySliceValue
Collaborator

Are there any types in Model that distinguishes between an array alloca and alloca? I think it would be preferable to have something like an ArrayPtr to replace the type of an ArraySliceValue.

Are there any types in `Model` that distinguishes between an array alloca and alloca? I think it would be preferable to have something like an `ArrayPtr` to replace the type of an `ArraySliceValue`.
Author
Collaborator

No. An array alloc's length is never coupled with the pointer.

I was doing something like ArraySliceValue before, but the length value didn't have much use and often ends up being redundant.

No. An array alloc's length is never coupled with the pointer. I was doing something like `ArraySliceValue` before, but the length value didn't have much use and often ends up being redundant.
derppening marked this conversation as resolved
@ -0,0 +195,4 @@
pub struct Instance<'ctx, M: Model<'ctx>> {
/// The model of this instance.
pub model: M,
/// The value of this instance.
Collaborator

nit: newline between fields

nit: newline between fields
derppening marked this conversation as resolved
@ -0,0 +57,4 @@
}
#[derive(Debug, Clone, Copy, Default)]
pub struct Float<N>(pub N);
Collaborator

Same as Array, bound N to FloatKind here as well.

Same as `Array`, bound `N` to `FloatKind` here as well.
@ -0,0 +80,4 @@
let exp_ty = self.0.get_float_type(generator, ctx);
// TODO: Inkwell does not have get_bit_width for FloatType?
Collaborator

No it does not, it has a limited set of supporting fp types.

No it does not, it has a limited set of supporting fp types.
derppening marked this conversation as resolved
@ -0,0 +35,4 @@
/// If `my_function_name` has not been declared in `ctx.module`, once `.returning()` is called, a function
/// declaration of `my_function_name` is added to `ctx.module`, where the [`FunctionType`] is deduced from
/// the argument types and returning type.
pub struct CallFunction<'ctx, 'a, 'b, 'c, 'd, G: CodeGenerator + ?Sized> {
Collaborator

I think FunctionCall or FuncCall might be a better name.

I think `FunctionCall` or `FuncCall` might be a better name.
Author
Collaborator

Sure

Sure
derppening marked this conversation as resolved
@ -0,0 +47,4 @@
}
impl<'ctx, 'a, 'b, 'c, 'd, G: CodeGenerator + ?Sized> CallFunction<'ctx, 'a, 'b, 'c, 'd, G> {
pub fn begin(generator: &'d mut G, ctx: &'b CodeGenContext<'ctx, 'a>, name: &'c str) -> Self {
Collaborator

I would name this builder instead.

I would name this `builder` instead.
Author
Collaborator

Unrelated: Is there any way to improve the situation about the lifetime signature for CallFunction? or is this unavoidable.

Unrelated: Is there any way to improve the situation about the lifetime signature for `CallFunction`? or is this unavoidable.
Collaborator

I did some testing and this might work:

pub struct CallFunction<'ctx, 'a, 'b, G: CodeGenerator + ?Sized> {
    generator: &'b mut G,
    ctx: &'b CodeGenContext<'ctx, 'a>,
    name: &'b str,  // Or alternatively String
    args: Vec<Arg<'ctx>>,
    attrs: Vec<&'static str>,
}

AFAICT this will work as long as the CallFunction instance is used in the same scope as it is declared, or name's lifetime extends until returning is invoked.

I did some testing and this might work: ```rs pub struct CallFunction<'ctx, 'a, 'b, G: CodeGenerator + ?Sized> { generator: &'b mut G, ctx: &'b CodeGenContext<'ctx, 'a>, name: &'b str, // Or alternatively String args: Vec<Arg<'ctx>>, attrs: Vec<&'static str>, } ``` AFAICT this will work as long as the `CallFunction` instance is used in the same scope as it is declared, or `name`'s lifetime extends until `returning` is invoked.
@ -0,0 +72,4 @@
/// Call the function and expect the function to return a value of type of `return_model`.
#[must_use]
pub fn returning<M: Model<'ctx>>(self, name: &str, return_model: M) -> Instance<'ctx, M> {
Collaborator

I don't really agree with the naming of the returning family of functions, as it doesn't indicate to the caller that by invoking returning it actually invokes the function as well.

I would suggest adding the return name and model to the begin function as an Option<(&str, Option<M>)>.

I don't really agree with the naming of the `returning` family of functions, as it doesn't indicate to the caller that by invoking `returning` it actually invokes the function as well. I would suggest adding the return name and model to the `begin` function as an `Option<(&str, Option<M>)>`.
Author
Collaborator

Passing Option<(&str, Option<M>)> to begin to define the return type may not be a good idea - If the function returns nothing, there is no type for M and Rust complains.

Passing `Option<(&str, Option<M>)>` to `begin` to define the return type may not be a good idea - If the function returns nothing, there is no type for `M` and Rust complains.
Collaborator

What about two functions, one that creates a void func and another that creates a value-returning func?

What about two functions, one that creates a void func and another that creates a value-returning func?
Author
Collaborator

If you are talking about something like begin_void and begin_with_return, sadly no.

It requires the return model M to somehow be propagated through the .arg().arg().arg() chain until, say, .finish_and_call() consuming the FnCall and returning an Instance<'ctx, M>.

This means that FnCall itself would have to hold M in its type.

So in the end M is still there even for begin_void, and there is no type for M.

If you are talking about something like `begin_void` and `begin_with_return`, sadly no. It requires the return model `M` to somehow be propagated through the `.arg().arg().arg()` chain until, say, `.finish_and_call()` consuming the `FnCall` and returning an `Instance<'ctx, M>`. This means that `FnCall` itself would have to hold `M` in its type. So in the end `M` is still there even for `begin_void`, and there is no type for `M`.
@ -0,0 +94,4 @@
}
#[derive(Debug, Clone, Copy, Default)]
pub struct Int<N>(pub N);
Collaborator

Same as with Float and Array.

Same as with `Float` and `Array`.
@ -0,0 +129,4 @@
}
impl<'ctx, N: IntKind<'ctx>> Int<N> {
pub fn const_int<G: CodeGenerator + ?Sized>(
Collaborator

What if I want a signed constant then?

What if I want a signed constant then?
Author
Collaborator

Oh I forgot to add another interface for it.

Oh I forgot to add another interface for it.
derppening marked this conversation as resolved
@ -0,0 +388,4 @@
}
#[must_use]
pub fn add(&self, ctx: &CodeGenContext<'ctx, '_>, other: Self) -> Self {
Collaborator

Why are operators provided by some functions but not by others?

Why are operators provided by some functions but not by others?
Author
Collaborator

I was adding the ones I needed to implement ndarrays. I figured I should add these ops on demand.

I was adding the ones I needed to implement ndarrays. I figured I should add these ops on demand.
@ -0,0 +110,4 @@
pub fn offset_const(
&self,
ctx: &CodeGenContext<'ctx, '_>,
offset: u64,
Collaborator

Same as Array - i64.

Same as `Array` - `i64`.
@ -0,0 +112,4 @@
ctx: &CodeGenContext<'ctx, '_>,
offset: u64,
) -> Instance<'ctx, Ptr<Item>> {
let offset = ctx.ctx.i32_type().const_int(offset, false);
Collaborator

i64_type, is_signed: true.

`i64_type`, `is_signed: true`.
@ -0,0 +198,4 @@
source: Self,
num_items: IntValue<'ctx>,
) {
// Force extend `num_items` and `itemsize` to `i64` so their types would match.
Collaborator

I would suggest extending them to SizeT to match the signature of memcpy in C.

I would suggest extending them to `SizeT` to match the signature of `memcpy` in C.
@ -0,0 +11,4 @@
use super::*;
/// A traveral that traverses a Rust `struct` that is used to declare an LLVM's struct's field types.
pub trait FieldTraversal<'ctx> {
Collaborator

MapStructFields? Since the only operations its implementing classes do is to add fields based on some criteria, matching what map does to an iterator. Or FieldMapper.

`MapStructFields`? Since the only operations its implementing classes do is to add fields based on some criteria, matching what `map` does to an iterator. Or `FieldMapper`.
@ -0,0 +13,4 @@
/// A traveral that traverses a Rust `struct` that is used to declare an LLVM's struct's field types.
pub trait FieldTraversal<'ctx> {
/// Output type of [`FieldTraversal::add`].
type Out<M>;
Collaborator

This name is non-descriptive. Output or OutType would be better.

This name is non-descriptive. `Output` or `OutType` would be better.
@ -0,0 +39,4 @@
}
/// A traversal to calculate the GEP index of fields.
pub struct GepFieldTraversal {
Collaborator

FieldIndexCollector? Same for below - Name those as *Collector?

`FieldIndexCollector`? Same for below - Name those as `*Collector`?
Author
Collaborator

This is a traversal that calculates the correct GEP index of each struct field.

struct NDArrayFields<F: FieldTraversal> {
  data: F::Out<Ptr<Int<Byte>>>,
  itemsize: F::Out<Int<SizeT>>,
  shape: F::Out<Ptr<Int<SizeT>>>,
  ...
}

NDArrayFields<GepFieldTraversal>::fields() yields:

NDArrayFields {
  data: GepField { gep_index: 0, ... },
  itemsize: GepField { gep_index: 1, ... },
  shape: GepField { gep_index: 2, ...},
  ...
}

And this object is used to enable the my_ndarray.gep(|f| f.data).set_index_const(0, my_value) syntax.

This is a traversal that calculates the correct GEP index of each struct field. ```rust struct NDArrayFields<F: FieldTraversal> { data: F::Out<Ptr<Int<Byte>>>, itemsize: F::Out<Int<SizeT>>, shape: F::Out<Ptr<Int<SizeT>>>, ... } ``` `NDArrayFields<GepFieldTraversal>::fields()` yields: ``` NDArrayFields { data: GepField { gep_index: 0, ... }, itemsize: GepField { gep_index: 1, ... }, shape: GepField { gep_index: 2, ...}, ... } ``` And this object is used to enable the `my_ndarray.gep(|f| f.data).set_index_const(0, my_value)` syntax.
@ -0,0 +41,4 @@
/// A traversal to calculate the GEP index of fields.
pub struct GepFieldTraversal {
/// The current GEP index.
gep_index_counter: u64,
Collaborator

u32.

When indexing into a (optionally packed) structure, only i32 integer constants are allowed

`u32`. > When indexing into a (optionally packed) structure, only i32 integer constants are allowed
@ -0,0 +100,4 @@
self.errors
.push(err.under_context(format!("field #{gep_index} '{name}'").as_str()));
}
} // Otherwise, it will be caught by Struct's `check_type`.
Collaborator

nit: newline

nit: newline
@ -0,0 +161,4 @@
/// let ndarray: Instance<'ctx, Ptr<F64NDArray>> = model.alloca(generator, ctx);
/// ```
///
/// ...and here is how you may manipulate/access `ndarray`:
Collaborator

Move this into the documentation for their respective functions. It is very difficult to refer to these examples when you are trying to look for how to use the APIs.

Move this into the documentation for their respective functions. It is very difficult to refer to these examples when you are trying to look for how to use the APIs.
@ -0,0 +192,4 @@
/// Traverse through all fields of this [`StructKind`].
///
/// Only used internally in this module for implementing other components.
fn traverse_fields<F: FieldTraversal<'ctx>>(&self, traversal: &mut F) -> Self::Fields<F>;
Collaborator

iter_fields

`iter_fields`
@ -0,0 +198,4 @@
///
/// Only used internally in this module for implementing other components.
fn fields(&self) -> Self::Fields<GepFieldTraversal> {
self.traverse_fields(&mut GepFieldTraversal { gep_index_counter: 0 })
Collaborator

Consider adding a new() function for GepFieldTraversal.

Consider adding a `new()` function for `GepFieldTraversal`.
@ -0,0 +207,4 @@
generator: &G,
ctx: &'ctx Context,
) -> StructType<'ctx> {
let mut traversal = TypeFieldTraversal { generator, ctx, field_types: Vec::new() };
Collaborator

Consider adding a new() function for this.

Consider adding a `new()` function for this.
@ -0,0 +258,4 @@
};
// Check each field individually.
let mut traversal = CheckTypeFieldTraversal {
Collaborator

Consider adding a new() function for this.

Consider adding a `new()` function for this.
Author
Collaborator

Are there Into implementations to convert Instance into BasicValue and Model into BasicType?

No, they could have been added but I just... didn't do that and kept using Instance::value for some reason. I can add them now.

The Traversal classes for Struct is really confusing me. Why is it necessary? Why couldn't it be made as a Vec and then use iter to collect information regarding each field?

I guess you mean there could have been a single visitor that collects the fields into a Vec<dyn Model> and do stuff with that Vec (like checking field types and more).

The short explanation is that I have tried, but Rust lifetimes were everywhere and it was quite unholy. I settled on this pattern and it works out fine, but I do not know of a better way to design this in Rust and play along with Rust's type system + without running into lifetime hell.

> Are there Into implementations to convert Instance into BasicValue and Model into BasicType? No, they could have been added but I just... didn't do that and kept using `Instance::value` for some reason. I can add them now. > The Traversal classes for Struct is really confusing me. Why is it necessary? Why couldn't it be made as a Vec and then use iter to collect information regarding each field? I guess you mean there could have been a single visitor that collects the fields into a `Vec<dyn Model>` and do stuff with that `Vec` (like checking field types and more). The short explanation is that I have tried, but Rust lifetimes were everywhere and it was quite unholy. I settled on this pattern and it works out fine, but I do not know of a better way to design this in Rust and play along with Rust's type system + without running into lifetime hell.
Collaborator

I guess you mean there could have been a single visitor that collects the fields into a Vec<dyn Model> and do stuff with that Vec (like checking field types and more).

The short explanation is that I have tried, but Rust lifetimes were everywhere and it was quite unholy. I settled on this pattern and it works out fine, but I do not know of a better way to design this in Rust and play along with Rust's type system + without running into lifetime hell.

What would a hypothetical lifetime-filled implementation look like?

> I guess you mean there could have been a single visitor that collects the fields into a `Vec<dyn Model>` and do stuff with that `Vec` (like checking field types and more). > > The short explanation is that I have tried, but Rust lifetimes were everywhere and it was quite unholy. I settled on this pattern and it works out fine, but I do not know of a better way to design this in Rust and play along with Rust's type system + without running into lifetime hell. What would a hypothetical lifetime-filled implementation look like?
sb10q closed this pull request 2024-08-27 22:50:15 +08:00
sb10q reopened this pull request 2024-08-27 22:51:23 +08:00
Author
Collaborator

What would a hypothetical lifetime-filled implementation look like?

I was trying to redo what I did that caused a mayhem of lifetimes, but I just came up a minor refactor to allow something like Vec<dyn Model> to exist and remove the complexity that FieldTraversal brings.

The branch https://git.m-labs.hk/M-Labs/nac3/src/branch/ndstrides-1-model-lifetime has almost all of your suggestions applied + the redesign. See 4e714cb53b for the exact commit that introduces the new design.

Basically, I was trying to have something like Vec<dyn Model>. Model itself is not object safe, I segregated the check_type part and get_type part out into ModelBase, and then everything else follows. With Vec<dyn ModelBase>, now I can implement StructKind::get_struct_type and Struct::check_type_impl nicely.

Should I use this instead of FieldTraversal?

> What would a hypothetical lifetime-filled implementation look like? I was trying to redo what I did that caused a mayhem of lifetimes, but I just came up a minor refactor to allow something like `Vec<dyn Model>` to exist and remove the complexity that `FieldTraversal` brings. The branch https://git.m-labs.hk/M-Labs/nac3/src/branch/ndstrides-1-model-lifetime has almost all of your suggestions applied + the redesign. See https://git.m-labs.hk/M-Labs/nac3/commit/4e714cb53b5c82f41a17b1e79a979c1e5a3010a2 for the exact commit that introduces the new design. Basically, I was trying to have something like `Vec<dyn Model>`. `Model` itself is not object safe, I segregated the `check_type` part and `get_type` part out into `ModelBase`, and then everything else follows. With `Vec<dyn ModelBase>`, now I can implement `StructKind::get_struct_type` and `Struct::check_type_impl` nicely. Should I use this instead of `FieldTraversal`?
Author
Collaborator

Update on

Are there Into implementations to convert Instance into BasicValue and Model into BasicType?

An Into for Model to BasicType is not possible because it requires extra arguments.

An Into for Instance to BasicValue is doable, but I am not sure if it will worth the "bloat" (in my view):

Note that trying to implement this is impossible:

/*
Conflicting implementations of trait `Into<_>` for type `model::core::Instance<'_, _>` conflicting implementation in crate `core`:
- impl<T, U> Into<U> for T
  where U: std::convert::From<T>;
*/
// impl<'ctx, V: BasicValue<'ctx>, M: Model<'ctx, Value = V>> Into<V> for Instance<'ctx, M> {
//     fn into(self) -> M {
//         todo!()
//     }
// }

So I must implement for all possible types of BasicValue variant their Into impl. e.g., Into<PointerValue<'ctx>> for Instance<'ctx, Ptr>, Into<IntValue<'ctx>> for Instance<'ctx, Int>, etc...

I suggest sticking to .value.

Update on > Are there Into implementations to convert `Instance` into `BasicValue` and `Model` into `BasicType`? An `Into` for `Model` to `BasicType` is not possible because it requires extra arguments. An `Into` for `Instance` to `BasicValue` is doable, but I am not sure if it will worth the "bloat" (in my view): Note that trying to implement this is impossible: ``` /* Conflicting implementations of trait `Into<_>` for type `model::core::Instance<'_, _>` conflicting implementation in crate `core`: - impl<T, U> Into<U> for T where U: std::convert::From<T>; */ // impl<'ctx, V: BasicValue<'ctx>, M: Model<'ctx, Value = V>> Into<V> for Instance<'ctx, M> { // fn into(self) -> M { // todo!() // } // } ``` So I must implement for all possible types of `BasicValue` variant their `Into` impl. e.g., `Into<PointerValue<'ctx>> for Instance<'ctx, Ptr>`, `Into<IntValue<'ctx>> for Instance<'ctx, Int>`, etc... I suggest sticking to `.value`.
lyken changed title from ndstrides: Introduce models to ndstrides #1: Introduce models 2024-08-28 13:24:15 +08:00
lyken changed title from ndstrides #1: Introduce models to ndstrides: [1] Introduce models 2024-08-28 13:24:27 +08:00
lyken force-pushed ndstrides-1-model from dfbbe66154 to d1c7a8ee50 2024-08-30 12:51:43 +08:00 Compare
lyken changed target branch from ndstrides-enhance-irrt to ndstrides 2024-08-30 12:59:39 +08:00
Author
Collaborator

Rebased ndstrides-1-model onto ndstrides; Branch ndstrides has been reset --hard to master. Note that the IRRT changes from ndstrides-enhance-irrt have been merged into master.

Rebased `ndstrides-1-model` onto `ndstrides`; Branch `ndstrides` has been `reset --hard` to `master`. Note that the IRRT changes from `ndstrides-enhance-irrt` have been merged into `master`.
derppening self-assigned this 2024-09-11 16:51:36 +08:00
derppening force-pushed ndstrides-1-model from d1c7a8ee50 to 668cad46b4 2024-10-04 15:07:50 +08:00 Compare
derppening force-pushed ndstrides-1-model from 668cad46b4 to 0bc98c447d 2024-10-04 15:22:15 +08:00 Compare
derppening force-pushed ndstrides-1-model from 0bc98c447d to 486cf68091 2024-10-17 13:44:38 +08:00 Compare
derppening force-pushed ndstrides-1-model from 486cf68091 to 499efa369c 2024-10-17 15:59:01 +08:00 Compare
derppening force-pushed ndstrides-1-model from 499efa369c to d9c7028761 2024-10-17 16:26:35 +08:00 Compare
derppening force-pushed ndstrides-1-model from d9c7028761 to 870a922367 2024-10-18 14:21:13 +08:00 Compare
Collaborator

After looking at the source code more closely, I think I will just refactor this entire changeset to use Proxy{Type,Value} instead.

The problem I have with the design of Model and Instance is that most of the functionality is provided by the Model class and its implementations, which IMO is not as good as operating directly on instances (think Int(Int32).s_extend(generator, ctx, value) vs value.s_extend(generator, ctx). There is also quite a bit of functionality overlap between Model and Proxy to the point where I doubt if everything can really just be implemented in terms of (augmented versions of) ProxyType and ProxyValue without introducing new classes and paradigms.

My plan is to refactor Proxy{Type,Value} to also be able to support LLVM primitive types, integrate any changes that will be useful (e.g. structure-related builders and traversers), create migration deprecation to move away from Model, and rebase the entire changeset.

After looking at the source code more closely, I think I will just refactor this entire changeset to use `Proxy{Type,Value}` instead. The problem I have with the design of `Model` and `Instance` is that most of the functionality is provided by the `Model` class and its implementations, which IMO is not as good as operating directly on instances (think `Int(Int32).s_extend(generator, ctx, value)` vs `value.s_extend(generator, ctx)`. There is also quite a bit of functionality overlap between `Model` and `Proxy` to the point where I doubt if everything can really just be implemented in terms of (augmented versions of) `ProxyType` and `ProxyValue` without introducing new classes and paradigms. My plan is to refactor `Proxy{Type,Value}` to also be able to support LLVM primitive types, integrate any changes that will be useful (e.g. structure-related builders and traversers), create migration deprecation to move away from `Model`, and rebase the entire changeset.
derppening closed this pull request 2024-11-25 16:03:53 +08:00

Pull request closed

Sign in to join this conversation.
No reviewers
No Milestone
No Assignees
2 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Reference: M-Labs/nac3#509
No description provided.