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

Remove reset instruction #1718

Open
wants to merge 19 commits into
base: main
Choose a base branch
from
Open

Remove reset instruction #1718

wants to merge 19 commits into from

Conversation

Schaeff
Copy link
Collaborator

@Schaeff Schaeff commented Aug 23, 2024

Remove the _reset instruction in the dispatcher.
When turning asm into pil, we have a dispatcher which first resets all registers and jumps to the correct part of the ROM. As a result, the write registers are unconstrained on the first row.

After this PR, registers are reset after calling return, just like for the pc. This removes the need for the _reset instruction, as long as we constrain the write registers to be 0 on the first row.

Here are the tradeoffs of this change:

  • pros
    • simplify the dispatcher, as now the pc and write registers are treated more similarly and we remove an instruction
    • fully constrain the write registers, including on the first row (note: this could also be achieved by constraining them to an arbitrary value on the first row)
    • save a main column instr__reset
    • save a ROM column p_instr__reset (negligible)
    • save a ROM row (negligible)
  • cons
    • adds a witness column per write register: because of the initialisation to 0 on the first row, the register update is now higher degree. Maybe this could be an intermediate column?

Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

⚠️ Performance Alert ⚠️

Possible performance regression was detected for benchmark 'Benchmarks'.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 1.20.

Benchmark suite Current: fec80a8 Previous: 0bdbbd2 Ratio
jit-benchmark/sqrt_879882356 3616 ns/iter (± 4) 2735 ns/iter (± 3) 1.32
jit-benchmark/sqrt_1882356 2861 ns/iter (± 12) 2165 ns/iter (± 4) 1.32
jit-benchmark/sqrt_1187956 2857 ns/iter (± 6) 2126 ns/iter (± 4) 1.34
jit-benchmark/sqrt_56 1687 ns/iter (± 2) 1261 ns/iter (± 1) 1.34

This comment was automatically generated by workflow using github-action-benchmark.

@Schaeff
Copy link
Collaborator Author

Schaeff commented Dec 19, 2024

I think the benchmark alert is a false positive because #2264 is out of sync with main. The regression was introduced in 8dd0a00

@Schaeff Schaeff marked this pull request as ready for review December 19, 2024 14:15
leonardoalt
leonardoalt previously approved these changes Dec 20, 2024
Copy link
Member

@leonardoalt leonardoalt left a comment

Choose a reason for hiding this comment

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

lgtm, but maybe you want someone else to also take a look?

@leonardoalt
Copy link
Member

has a bunch of conflicts now =/

@lvella
Copy link
Member

lvella commented Feb 26, 2025

Did you base this PR off some other PR that was merged? Then you need to merge main back into the branch of that base PR, then merge that branch into the branch of this PR, and all the conflicts and spurious commits will magically disappear...

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.

4 participants