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

fix: Print comptime::Value better #5655

Merged
merged 4 commits into from
Aug 2, 2024

Conversation

jfecher
Copy link
Contributor

@jfecher jfecher commented Jul 31, 2024

Description

Problem*

Summary*

Several Value variants are just ids which we previously couldn't print well. I've added a helper type to hold a NodeInterner so that we can print the names of these values as well. This will now print, e.g. Eq instead of (trait definition)

Additional Context

Documentation*

Check one:

  • No documentation needed.
  • Documentation included in this PR.
  • [For Experimental Features] Documentation to be submitted in a separate PR.

PR Checklist*

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

@jfecher jfecher requested a review from a team July 31, 2024 21:03
Copy link
Contributor

github-actions bot commented Jul 31, 2024

Changes to circuit sizes

Generated at commit: 4b462906282cf8ea05bbbe8a4490a7f6efed1dfd, compared to commit: e592a10d9ef76653007cc28aade4852981a48827

🧾 Summary (10% most significant diffs)

Program ACIR opcodes (+/-) % Circuit size (+/-) %
import +2 ❌ +40.00% +1 ❌ +11.11%
strings +2 ❌ +5.26% -11,578 ✅ -80.55%

Full diff report 👇
Program ACIR opcodes (+/-) % Circuit size (+/-) %
import 7 (+2) +40.00% 10 (+1) +11.11%
merkle_insert 1,803 (+24) +1.35% 29,032 (0) 0.00%
pedersen_commitment 8 (-3) -27.27% 28,742 (0) 0.00%
pedersen_hash 6 (+4) +200.00% 28,742 (0) 0.00%
simple_shield 53 (+4) +8.16% 29,032 (0) 0.00%
pedersen_check 24 (-14) -36.84% 28,829 (-29) -0.10%
schnorr 524 (+4) +0.77% 54,330 (-58) -0.11%
strings 40 (+2) +5.26% 2,795 (-11,578) -80.55%

Copy link
Collaborator

@asterite asterite left a comment

Choose a reason for hiding this comment

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

LGTM

Is there a way to test this? (maybe it's tricky because this would be the output of a println call?)

@jfecher
Copy link
Contributor Author

jfecher commented Aug 1, 2024

@asterite not that I know of besides testing it manually.

I'm surprised there are circuit size changes here at all though. Especially considering this is just changing the comptime values printing and not runtime ones.

@asterite
Copy link
Collaborator

asterite commented Aug 1, 2024

Ah, you are right.

I just checked out the branch this PR is on top of, jf/quoted-to-trait-constraints, and I see the bump in size also happens there. I don't know why a message wasn't posted in that PR, though.

@asterite
Copy link
Collaborator

asterite commented Aug 1, 2024

Nevermind, now I don't get that difference in that other PR... 🤔

@asterite
Copy link
Collaborator

asterite commented Aug 1, 2024

Oh, and merging this branch against master makes it go down again... I think. We'll have to wait for the other PR to get merged, then merge against master.

@vezenovm
Copy link
Contributor

vezenovm commented Aug 1, 2024

Oh, and merging this branch against master makes it go down again... I think. We'll have to wait for the other PR to get merged, then merge against master.

Yeah I think so as well. If you look in the Report gates diff action you will see Downloading artifact "jf-quoted-to-trait-constraints.gates_report.json" of repository "noir-lang/noir" with ID "1761974290" under the Compare gates report step.

# Description

## Problem\*

Resolves <!-- Link to GitHub Issue -->

## Summary\*

We stored `comptime_scopes` in the Elaborator, but elaborators are
created one per each crate. So any globals from other crates would be
invisible to each next crate, or their value would be reset. I've moved
`comptime_scopes` to the NodeInterner to fix this.

## Additional Context

With this, `derive` now works in the stdlib. I've decided to split
adding that into a separate PR though.

## Documentation\*

Check one:
- [x] No documentation needed.
- [ ] Documentation included in this PR.
- [ ] **[For Experimental Features]** Documentation to be submitted in a
separate PR.

# PR Checklist\*

- [x] I have tested the changes locally.
- [x] I have formatted the changes with [Prettier](https://prettier.io/)
and/or `cargo fmt` on default settings.
@jfecher jfecher merged commit 081101b into jf/quoted-to-trait-constraints Aug 2, 2024
23 checks passed
@jfecher jfecher deleted the jf/value-print branch August 2, 2024 14:09
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