Skip to content

Constant evaluation of to_bits()#464

Merged
guipublic merged 5 commits intomasterfrom
gd/intrinsic_eval
Nov 16, 2022
Merged

Constant evaluation of to_bits()#464
guipublic merged 5 commits intomasterfrom
gd/intrinsic_eval

Conversation

@guipublic
Copy link
Contributor

@guipublic guipublic commented Nov 11, 2022 •
edited
Loading

Description

Summary of changes

This PR fix a weird issue found by a user that is in fact two issues:

  • dominator relation was overwritten for IF join block, we need to use the dominator instead of setting the dominated
  • to_bits() constant evaluation was writing directly the result instead of creating the relevant store instructions which caused the CSE to loose the value after merging IF branches.

Test additions / changes

Regression test case is added to 9_conditional integration test.

Fixed issue

CSE was mixing instructions from IF branches because of wrong dominator relation for the join and bad evaluation of to_bits() with constant arguments.
See the provided test case for a repro.

Resolves (link to issue)

Checklist

  • I have tested the changes locally.
  • I have formatted the changes with cargo fmt with default settings.
  • I have described the issue that this PR solved and added a regression test case
  • I have reviewed the changes on GitHub, line by line.
  • I have ensured all changes are covered in the description.

vezenovm
vezenovm reviewed Nov 15, 2022
View reviewed changes
Copy link
Contributor

@vezenovm vezenovm left a comment

Choose a reason for hiding this comment

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

LGTM. Just looks like there is still one merge conflict. Once that is fixed I can approve

@guipublic guipublic merged commit 0775b8d into master Nov 16, 2022
@guipublic guipublic deleted the gd/intrinsic_eval branch November 16, 2022 10:06
TomAFrench added a commit to TomAFrench/noir that referenced this pull request Nov 16, 2022
@TomAFrench
Merge branch 'master' into separate-arg-collection-from-proving
2ce3d59 
* master:
  Fix conditional with array parameter (noir-lang#475)
  Constant evaluation of to_bits() (noir-lang#464)
  propagate error from cse (noir-lang#478)
  use return value only when witness are all solved (noir-lang#473)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@vezenovm vezenovm vezenovm left review comments

@jfecher jfecher jfecher approved these changes

@kevaundray kevaundray Awaiting requested review from kevaundray

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@guipublic @jfecher @vezenovm