-
Notifications
You must be signed in to change notification settings - Fork 43
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
Exposing vector_add to zok frontend #78
base: master
Are you sure you want to change the base?
Conversation
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.
A few comments so far. I'll have another look once you've got edits to the constant folder!
// vector functions | ||
def vector_add<N>(u32[N] a, u32[N] b) -> u32[N]: | ||
return [0; N] |
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.
I think if you're only going to support one type here it should be field
, not u32
.
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.
I think only BV are currently supported in the FHE backend, which is why I left it as u32.
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.
Still, it might make sense to support all the basic types, no? It wouldn't be much extra work:
- add vadd_field, vadd_u64, vadd_u32, vadd_u16, vadd_u8 stubs in EMBED
- in builtin_call_, extend the current match clause to catch all of these
- extend the function in
zsharp/term.rs
to support both bitvector and field types - the current constant folder may or may not work, but I bet there's a small edit distance to working if no
Since this is so simple, I don't see a reason to leave it half-done. But I could be missing something!
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.
I see, updated!
src/ir/opt/cfold.rs
Outdated
let mut new_map: BTreeMap<Value, Value> = BTreeMap::new(); | ||
for (a_ind, a_val) in &a.map { | ||
let b_val = &b.map[a_ind]; | ||
let res = fold_cache( |
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.
@kwantam How would I evaluate the operation on va
and vb
without the recursive call to fold_cache here?
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.
High-level answer: the Op::Map(op)
match clause starting on line 248 should push a new term onto the stack that is equivalent to the term containing the Op::Map(_)
, but which eliminates the map. I can give you more details later.
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.
a few more comments
src/ir/opt/cfold.rs
Outdated
let mut new_map: BTreeMap<Value, Value> = BTreeMap::new(); | ||
for (a_ind, a_val) in &a.map { | ||
let b_val = &b.map[a_ind]; | ||
let res = fold_cache( |
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.
High-level answer: the Op::Map(op)
match clause starting on line 248 should push a new term onto the stack that is equivalent to the term containing the Op::Map(_)
, but which eliminates the map. I can give you more details later.
Sorry for the delay. I'll spend some time reviewing this today, I hope. |
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.
I left a few comments. Happy to jump on a call and discuss in more detail. The upshot is, the functionality looks mostly right but what's currently implemented is unnecessarily inefficient.
No description provided.