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

Exposing vector_add to zok frontend #78

Open
wants to merge 16 commits into
base: master
Choose a base branch
from
Open

Exposing vector_add to zok frontend #78

wants to merge 16 commits into from

Conversation

edwjchen
Copy link
Collaborator

No description provided.

@edwjchen edwjchen requested a review from kwantam April 26, 2022 17:26
Copy link
Collaborator

@kwantam kwantam left a 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!

scripts/zx_tests/vector_add.zx Outdated Show resolved Hide resolved
src/front/zsharp/mod.rs Outdated Show resolved Hide resolved
src/front/zsharp/mod.rs Outdated Show resolved Hide resolved
src/front/zsharp/term.rs Outdated Show resolved Hide resolved
Comment on lines 79 to 81
// vector functions
def vector_add<N>(u32[N] a, u32[N] b) -> u32[N]:
return [0; N]
Copy link
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
Collaborator Author

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
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
Collaborator Author

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 kwantam April 27, 2022 22:22
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
Collaborator Author

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
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.

Copy link
Collaborator

@kwantam kwantam left a 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

Cargo.lock Outdated Show resolved Hide resolved
src/front/zsharp/mod.rs Outdated Show resolved Hide resolved
src/front/zsharp/mod.rs Outdated Show resolved Hide resolved
src/front/zsharp/term.rs Outdated Show resolved Hide resolved
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
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.

src/ir/opt/cfold.rs Outdated Show resolved Hide resolved
@kwantam
Copy link
Collaborator

kwantam commented May 7, 2022

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

Copy link
Collaborator

@kwantam kwantam left a 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.

src/front/zsharp/term.rs Outdated Show resolved Hide resolved
src/ir/opt/cfold.rs Outdated Show resolved Hide resolved
src/ir/opt/cfold.rs Outdated Show resolved Hide resolved
src/ir/opt/cfold.rs Outdated Show resolved Hide resolved
src/ir/opt/cfold.rs Outdated Show resolved Hide resolved
src/ir/term/mod.rs Outdated Show resolved Hide resolved
src/ir/term/mod.rs Outdated Show resolved Hide resolved
src/ir/term/mod.rs Outdated Show resolved Hide resolved
src/ir/term/mod.rs Outdated Show resolved Hide resolved
src/ir/term/mod.rs Outdated Show resolved Hide resolved
Sign up for free to 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