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

chore(ssa refactor): Make abi generation completely deterministic in the function signature #1426

Closed
wants to merge 3 commits into from

Conversation

kevaundray
Copy link
Contributor

Description

Related to #1425 -- This means that Abi generation is completely determinsitic in the function signature. Instead of the return witnesses depending on the body of main.

Problem*

Resolves

Summary*

This PR sets out to

Example

Before:


After:


Documentation

  • This PR requires documentation updates when merged.

    • I will submit a noir-lang/docs PR.
    • I will request for and support Dev Rel's help in documenting this PR.

Additional Context

PR Checklist*

  • I have tested the changes locally.
  • I have formatted the changes with Prettier and/or cargo fmt on default settings.

@kevaundray kevaundray requested a review from joss-aztec May 27, 2023 14:58
@kevaundray
Copy link
Contributor Author

Since the distinct keyword possibly applies constraints to enforce distinctness, this may make this unpleasant -- It depends on how we approach the initial issue of duplicated code in two places

@kevaundray
Copy link
Contributor Author

Converting to Draft as this is not high priority and we can come back to it once we implement the distinct keyword

@kevaundray kevaundray marked this pull request as draft May 29, 2023 18:21
Copy link
Contributor

@joss-aztec joss-aztec left a comment

Choose a reason for hiding this comment

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

To fix this implementation you would have to do something akin to reserving witness indices for the result, and then coming back to convert the corresponding values later.


for instruction_id in entry_block.instructions() {
self.convert_ssa_instruction(*instruction_id, dfg);
}

self.convert_ssa_return(entry_block.terminator().unwrap(), dfg);

Copy link
Contributor

@joss-aztec joss-aztec May 30, 2023

Choose a reason for hiding this comment

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

Moving convert_ssa_return to before all the instructions have been converted causes this to crash, because it will attempt to fetch instruction results before they have been defined.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We call convert_ssa_value in the convert_ssa_return so it should create the AcirVar if it does not exist. This is needed for things like when you just return a constant which would not have been defined before in the program and therefore not cached

@kevaundray
Copy link
Contributor Author

@joss-aztec Lets come back to this once distinct is implemented

@TomAFrench
Copy link
Member

I don't see how we can have both:

  1. return witness indices are known prior to ACIR generation/optimization
  2. redundant witnesses in the return values are merged.

Number 2. requires knowledge of the ACIR in order to determine whether we can tell if two witnesses are equal at compile-time. Are we then throwing this away in general rather than having the distinct keyword then?

@TomAFrench
Copy link
Member

@kevaundray are we ok to close this?

@TomAFrench TomAFrench closed this Aug 21, 2023
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.

3 participants