-
Notifications
You must be signed in to change notification settings - Fork 138
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(hlapi): add Array types #1089
base: main
Are you sure you want to change the base?
Conversation
4374ba3
to
d385138
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The general construction of backends looks straight forward with the current trait design, I'm a bit worried about the complexity of the trait bounds, though it might not really be an issue if those are meant to stay internal, but even then it might be hard to have other people onboarded on that code. Also sometimes the rust trait solvers chokes on more intricate constructs.
I don't know if it's doable but as core crypto containers are basically aliases for AsRef/AsMut, I'm wondering if we are not missing something somewhere to get the array like structs down to slices, so that all the combinations of BitAnd trait bounds e.g. would reduce to something like ArrayLikeBitAnd<LhsArrayLike, RhsArrayLike> where the array like arguments can reduce to slices and therefore have a single bound, maybe it's a matter of trait aliases/super traits we could discuss when there is some time to do so.
I'm a bit curious as to what the GPU stuff would look like in the Dyn backend
/// This trait is for "Owned" array types | ||
pub trait IOwnedArray: Clone + Slicing + SlicingMut {} | ||
|
||
/// Internal trait to abstract how container store data for the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if the trait is internal does it need to be pub ?
I was just curious given the Slicing traits above
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It needs to be pub because it appears in trait bounds of pub functions
but it being pub does not mean users can actually implement it since its in a private module
pub trait Slicing { | ||
type Slice<'a> | ||
where | ||
Self: 'a; | ||
|
||
fn slice(&self, range: impl RangeBounds<usize>) -> Self::Slice<'_>; | ||
|
||
fn as_slice(&self) -> Self::Slice<'_> { | ||
self.slice(..) | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
from a high level point of view I currently don't quite make out the difference between the slicing primitives here and the ones for backends lower in the file, also naming conventions seem to slightly differ between the two though they seem to generally do the same thing from afar
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ideally, I would like to be able to have
pub trait Slicing {
type Slice<'a>
where
Self: 'a;
fn slice(&self, range: impl RangeBounds<usize>) -> Self::Slice<'_>;
fn as_slice(&self) -> Self::Slice<'_> {
self.slice(..)
}
}
pub trait BackendDataContaier: Slicing {
type Backend: ArrayBackend;
}
but, then I need to express that Slicing::Slice == <Backend as ArrayBackend>::Slice
which is something normally easy', but due to the fact that I use GAT (type Slice<'a> where Self: 'a
) it becomes a nightmare / impossible as neither of the below things work
pub trait BackendDataContaier: Slicing {
type Backend: for <'a> ArrayBackend<Slice<'a> = Slicing::Slice<'a>>;
}
pub trait BackendDataContaier<'a>: Slicing {
type Backend: ArrayBackend<Slice<'a> = Slicing::Slice<'a>>;
}
+ BitOr<Array, Output = Array> | ||
+ for<'a> BitOr<&'a Array, Output = Array>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
might be interesting to do some super traits for those if possible to have a single "simple" bound for BitOr, unless we don't expect users to ever use those
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes I also thought about something like this :
https://github.com/zama-ai/tfhe-rs-internal/issues/488#issue-2167005871
+ BitAnd<FheBackendArray<Backend, Id>, Output = FheBackendArray<Backend, Id>> | ||
+ for<'a> BitAnd<&'a FheBackendArray<Backend, Id>, Output = FheBackendArray<Backend, Id>>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same remark as below (to have some super trait if possible) though here the bounds look even more complex
Something like ArrayArrayBitAnd, ArraySliceBitAnd (maybe symetric to manage SliceArray as well with e.g. an alias)
This design very much like makes me think of the container stuff from core the approach is to:
- take things that can be turned into slices
- always use slices to abstract over the original container
I might be missing something and am sure the bounds are like this for a good reason but they look crazy 😅
If external users are never meant to fiddle with those it might be ok, but it still looks heavy and the rust trait solver sometimes chokes on more elaborate usage
|
||
pub struct DynFheBoolArrayBackend; | ||
|
||
impl ArrayBackend for DynFheBoolArrayBackend { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the dyn backend is the one that could mix devices right ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The one where you can select and change device at runtime
The problem with AsRef, AsMut, Borrow, BorrowMut family of traits is that they do not work with struct where the reference is internal to the type since they require you to return some kind of In the cuda backend (and maybe other backends) slice type will likely need to be defined as struct CudaSlice<'a>{
ptr: *mu c_void, // non owning ptr to contiguous memory chunk
len: usize,
ref: PhantomData<&'a ()>,
} Here, the reference is 'internal' / inside of the type as opposed as the CPU side where the slice type is a primitive type So on the cuda side, we will have a pub trait AsRef<T>
where
T: ?Sized {
// Required method
fn as_ref(&self) -> &T;
} because struct CudaVec {
cuda_ptr: *mut c_void,
len: usize,
}
impl<'a> AsRef<CudaSlice<'a>> for CudaVec {
fn as_ref(&self> &CudaSlice<'a> {
let slice = CudaSlice{ptr: self.ptr, len: self.len, ref: PhantomData};
&slice // Ooopsy
}
} |
I know the AsRef/AsMut cannot work because they force to return a ref/ref mut was wondering if the current design is the closest to that behavior (converting to slice types always) without returning refs |
fn bitnot(lhs: Self::Slice<'_>) -> Self::Owned; | ||
} | ||
|
||
/// Trait for backends that can do bitwise operations |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
with clear elements
pub trait BackendDataContainer { | ||
type Backend: ArrayBackend; | ||
|
||
fn as_sub_slice( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rename slice
to be more consistent with trait Slicing
Same for mut
variant
type SliceMut<'a>: BackendDataContainerMut<Backend = Self> | ||
where | ||
Self: 'a; | ||
type Owned: BackendDataContainerMut<Backend = Self>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
BackendDataContainerMut
for Owned
, is this an error?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, BackendDataContainerMut
is meant to be impl on type where we can mutate the data that is contained, which is something SlicesMut and Owned containers can do
d385138
to
360707b
Compare
360707b
to
0a4de3e
Compare
No description provided.