Skip to content

Exposing vector_add to zok frontend - #78

Open
edwjchen wants to merge 16 commits into
masterfrom
vector_add
Open

Exposing vector_add to zok frontend#78
edwjchen wants to merge 16 commits into
masterfrom
vector_add

Conversation

@edwjchen

Copy link
Copy Markdown
Collaborator

No description provided.

@edwjchen
edwjchen requested a review from kwantamApril 26, 2022 17:26

@kwantamkwantam left a comment

Copy link
Copy Markdown
Collaborator

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!

Comment threadscripts/zx_tests/vector_add.zx Outdated
Comment threadsrc/front/zsharp/mod.rs Outdated
Comment threadsrc/front/zsharp/mod.rs Outdated
Comment threadsrc/front/zsharp/term.rs Outdated
Comment on lines +79 to +81
// vector functions
def vector_add<N>(u32[N] a, u32[N] b) -> u32[N]:
return [0; N] No newline at end of file

Copy link
Copy Markdown
Collaborator

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.

Copy link
Copy Markdown
CollaboratorAuthor

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.

Copy link
Copy Markdown
Collaborator

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!

Copy link
Copy Markdown
CollaboratorAuthor

Choose a reason for hiding this comment

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

I see, updated!

@edwjchen
edwjchen requested a review from kwantamApril 27, 2022 22:22
Comment threadsrc/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(

Copy link
Copy Markdown
CollaboratorAuthor

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?

Copy link
Copy Markdown
Collaborator

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.

@kwantamkwantam left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

a few more comments

Comment threadCargo.lock Outdated
Comment threadsrc/front/zsharp/mod.rs Outdated
Comment threadsrc/front/zsharp/mod.rs Outdated
Comment threadsrc/front/zsharp/term.rs Outdated
Comment threadsrc/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(

Copy link
Copy Markdown
Collaborator

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.

Comment threadsrc/ir/opt/cfold.rs Outdated
@kwantam

Copy link
Copy Markdown
Collaborator

Sorry for the delay. I'll spend some time reviewing this today, I hope.

@kwantamkwantam left a comment

Copy link
Copy Markdown
Collaborator

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.

Comment threadsrc/front/zsharp/term.rs Outdated
Comment threadsrc/ir/opt/cfold.rs Outdated
Comment threadsrc/ir/opt/cfold.rs Outdated
Comment threadsrc/ir/opt/cfold.rs Outdated
Comment threadsrc/ir/opt/cfold.rs Outdated
Comment threadsrc/ir/term/mod.rs Outdated
Comment threadsrc/ir/term/mod.rs Outdated
Comment threadsrc/ir/term/mod.rs Outdated
Comment threadsrc/ir/term/mod.rs Outdated
Comment threadsrc/ir/term/mod.rs Outdated
Sign up for freeto 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

@edwjchen@kwantam