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

Check multiplicities in mock prover #2310

Merged
merged 7 commits into from
Jan 13, 2025
Merged

Check multiplicities in mock prover #2310

merged 7 commits into from
Jan 13, 2025

Conversation

Schaeff
Copy link
Collaborator

@Schaeff Schaeff commented Jan 7, 2025 •
edited
Loading

Check multiplicities in the mock prover:

  • when checking a connection, we count the multiplicities on the caller and callee
    • if the connection is a PhantomLookup, this involves the multiplicity column. In this case, the multiplicities are checked to match.
    • otherwise, we count each callee tuple with multiplicity 1. In this case, the multiplicities will probably not match, and that's ok, so we do not check them to be equal.

@georgwiese georgwiese mentioned this pull request Jan 9, 2025
Merged
github-merge-queue bot pushed a commit that referenced this pull request Jan 9, 2025
@georgwiese
Do multiplicity witgen in separate stage (#2319)
e328eb9 
While working on #2306, @Schaeff came across several bugs in
multiplicity witness generation. These were undetected, because we
ignored multiplicities in the mock prover, which will be fixed by #2310.
With this PR, #2310 will be green.

The issue was that counting multiplicities inside
`Machine::process_plookup()` fails if the caller actually discards the
result. This happens in a few places, for example during our loop
optimization in the "dynamic machine".

With this PR, we instead have a centralized
`MultiplicityColumnGenerator` that counts multiplicities after the fact,
by going over each lookup, evaluating the two selected tuples on all
rows, and counting how often each element in the LHS appears in the RHS.

To measure the runtime of this, I ran:
```sh
export TEST=keccak
export POWDR_JIT_OPT_LEVEL=0

cargo run -r --bin powdr-rs compile riscv/tests/riscv_data/$TEST -o output --max-degree-log 18
cargo run -r --features plonky3,halo2 pil output/$TEST.asm -o output -f --field gl --linker-mode bus
```

I get the following profile on the server:
```
 == Witgen profile (2554126 events)
   32.4% (    2.6s): Secondary machine 0: main_binary (BlockMachine)
   23.1% (    1.9s): Main machine (Dynamic)
   12.7% (    1.0s): Secondary machine 4: main_regs (DoubleSortedWitnesses32)
   10.0% ( 809.9ms): FixedLookup
    7.7% ( 621.1ms): Secondary machine 5: main_shift (BlockMachine)
    5.6% ( 454.6ms): Secondary machine 2: main_poseidon_gl (BlockMachine)
    3.8% ( 312.3ms): multiplicity witgen
    3.8% ( 308.2ms): witgen (outer code)
    0.6% (  45.3ms): Secondary machine 1: main_memory (DoubleSortedWitnesses32)
    0.4% (  33.4ms): Secondary machine 6: main_split_gl (BlockMachine)
    0.0% (   8.0µs): Secondary machine 3: main_publics (WriteOnceMemory)
  ---------------------------
    ==> Total: 8.114630092s
```

So the cost is ~4%. I'm sure it can be optimized further but I would
like to leave this to a future PR.
github-actions[bot]

This comment was marked as outdated.

Sign in to view

leonardoalt pushed a commit that referenced this pull request Jan 11, 2025
@georgwiese @leonardoalt
Do multiplicity witgen in separate stage (#2319)
11a8ab6 
While working on #2306, @Schaeff came across several bugs in
multiplicity witness generation. These were undetected, because we
ignored multiplicities in the mock prover, which will be fixed by #2310.
With this PR, #2310 will be green.

The issue was that counting multiplicities inside
`Machine::process_plookup()` fails if the caller actually discards the
result. This happens in a few places, for example during our loop
optimization in the "dynamic machine".

With this PR, we instead have a centralized
`MultiplicityColumnGenerator` that counts multiplicities after the fact,
by going over each lookup, evaluating the two selected tuples on all
rows, and counting how often each element in the LHS appears in the RHS.

To measure the runtime of this, I ran:
```sh
export TEST=keccak
export POWDR_JIT_OPT_LEVEL=0

cargo run -r --bin powdr-rs compile riscv/tests/riscv_data/$TEST -o output --max-degree-log 18
cargo run -r --features plonky3,halo2 pil output/$TEST.asm -o output -f --field gl --linker-mode bus
```

I get the following profile on the server:
```
 == Witgen profile (2554126 events)
   32.4% (    2.6s): Secondary machine 0: main_binary (BlockMachine)
   23.1% (    1.9s): Main machine (Dynamic)
   12.7% (    1.0s): Secondary machine 4: main_regs (DoubleSortedWitnesses32)
   10.0% ( 809.9ms): FixedLookup
    7.7% ( 621.1ms): Secondary machine 5: main_shift (BlockMachine)
    5.6% ( 454.6ms): Secondary machine 2: main_poseidon_gl (BlockMachine)
    3.8% ( 312.3ms): multiplicity witgen
    3.8% ( 308.2ms): witgen (outer code)
    0.6% (  45.3ms): Secondary machine 1: main_memory (DoubleSortedWitnesses32)
    0.4% (  33.4ms): Secondary machine 6: main_split_gl (BlockMachine)
    0.0% (   8.0µs): Secondary machine 3: main_publics (WriteOnceMemory)
  ---------------------------
    ==> Total: 8.114630092s
```

So the cost is ~4%. I'm sure it can be optimized further but I would
like to leave this to a future PR.
@Schaeff Schaeff marked this pull request as ready for review January 13, 2025 10:03
@Schaeff Schaeff marked this pull request as draft January 13, 2025 14:44
@Schaeff Schaeff marked this pull request as ready for review January 13, 2025 15:57
georgwiese
georgwiese approved these changes Jan 13, 2025
View reviewed changes
Copy link
Collaborator

@georgwiese georgwiese left a comment

Choose a reason for hiding this comment

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

Nice!

@Schaeff Schaeff added this pull request to the merge queue Jan 13, 2025
Merged via the queue into main with commit f795d41 Jan 13, 2025
16 checks passed
@Schaeff Schaeff deleted the check-multiplicities-in-mock-prover branch January 13, 2025 17:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@github-actions github-actions[bot] github-actions[bot] left review comments

@georgwiese georgwiese georgwiese approved these changes

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

Successfully merging this pull request may close these issues.
2 participants
@Schaeff @georgwiese