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.

Sign up for GitHub

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

Jump to bottom

Make the trace reference more general in the IR #92

Merged
merged 2 commits into from
Dec 5, 2022
Merged

Make the trace reference more general in the IR #92

merged 2 commits into from
Dec 5, 2022

Conversation

grjte
Copy link
Contributor

@grjte grjte commented Dec 2, 2022

This PR makes the trace reference more general in the IR, addressing issue #54. This PR depends on the fix in #91 and should not be merged first.

The IR has been generalized everywhere except:

  • the insertion of random values into the graph. This can't change until we have a way to describe different sets of randomness in the AirScript syntax.
  • the way we store boundary constraints. This needs more thinking and probably deserves its own issue

@grjte grjte force-pushed the grjte-ir-trace-ref branch from b04a3f1 to f93a0ab Compare December 2, 2022 23:53
@grjte grjte changed the title Grjte ir trace ref Make the trace reference more general in the IR Dec 2, 2022
@grjte grjte marked this pull request as ready for review December 2, 2022 23:59
bobbinth
bobbinth approved these changes Dec 3, 2022
View reviewed changes
Copy link
Contributor

@bobbinth bobbinth left a comment

Choose a reason for hiding this comment

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

Looks good! Thank you! I left a few non-blocking comments inline.

codegen/winterfell/src/air/mod.rs Outdated Show resolved Hide resolved
codegen/winterfell/src/air/mod.rs Outdated Show resolved Hide resolved
ir/src/symbol_table.rs Outdated Show resolved Hide resolved
ir/src/transition_constraints/mod.rs Outdated Show resolved Hide resolved
tohrnii
tohrnii approved these changes Dec 3, 2022
View reviewed changes
Copy link
Contributor

@tohrnii tohrnii left a comment

Choose a reason for hiding this comment

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

Thank you @grjte! Looks great! I left a few very minor nits inline.

codegen/winterfell/src/air/transition_constraints.rs Outdated Show resolved Hide resolved
ir/src/boundary_constraints.rs Show resolved Hide resolved
ir/src/symbol_table.rs Outdated Show resolved Hide resolved
@grjte grjte force-pushed the grjte-ir-trace-ref branch from f93a0ab to c9963a7 Compare December 3, 2022 12:05
Al-Kindi-0
Al-Kindi-0 approved these changes Dec 3, 2022
View reviewed changes
Copy link
Collaborator

@Al-Kindi-0 Al-Kindi-0 left a comment

Choose a reason for hiding this comment

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

Looks great, thank you @grjte !

@grjte grjte force-pushed the grjte-ir-trace-ref branch from c9963a7 to 342e049 Compare December 3, 2022 18:14
@tohrnii tohrnii mentioned this pull request Dec 5, 2022
Merged
14 tasks
@bobbinth bobbinth changed the base branch from next to grjte-fix-aux-codegen December 5, 2022 08:15
@tohrnii tohrnii merged commit c14eedf into grjte-fix-aux-codegen Dec 5, 2022
This was referenced Dec 5, 2022
Merged
Closed
@tohrnii tohrnii deleted the grjte-ir-trace-ref branch December 8, 2022 11:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tohrnii tohrnii tohrnii approved these changes

@Al-Kindi-0 Al-Kindi-0 Al-Kindi-0 approved these changes

@bobbinth bobbinth bobbinth approved these changes

@Fumuran Fumuran Awaiting requested review from Fumuran

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@grjte @bobbinth @Al-Kindi-0 @tohrnii