Enable Bus in tests #2289

georgwiese · 2024-12-30T09:53:50Z

This PR uses LinkerMode::Bus in all tests which:

Are on Goldilocks or BN254 (the bus is not implemented for smaller fields), AND
Are tested with the mock prover or Plonky3 (while Halo2 does support the bus in principle, its runtime is exponential, because it inlines intermediate polynomials)

This is true for most tests.

Doing so, I came across a few bugs. I added comments for them below.

leonardoalt · 2024-12-30T10:00:08Z

pipeline/tests/asm.rs

@@ -22,7 +22,8 @@ fn slice_to_vec<T: FieldElement>(arr: &[i32]) -> Vec<T> {
 fn sqrt_asm() {
    let f = "asm/sqrt.asm";
    let i = [3];
-    let pipeline: Pipeline<GoldilocksField> = make_prepared_pipeline(f, slice_to_vec(&i), vec![]);
+    let pipeline: Pipeline<GoldilocksField> =
+        make_prepared_pipeline(f, slice_to_vec(&i), vec![], LinkerMode::Bus);


So these are only testing with bus mode?

Yes, I think it should be the default now, whenever possible.

leonardoalt · 2024-12-30T10:01:16Z

pipeline/tests/pil.rs

@@ -110,7 +111,8 @@ fn fibonacci() {
 fn fibonacci_with_public() {
    // Public references are not supported by the backends yet, but we can test witness generation.
    let f = "pil/fibonacci_with_public.pil";
-    let mut pipeline: Pipeline<GoldilocksField> = make_prepared_pipeline(f, vec![], vec![]);
+    let mut pipeline: Pipeline<GoldilocksField> =
+        make_prepared_pipeline(f, vec![], vec![], LinkerMode::Bus);


Why only bus?

georgwiese · 2024-12-30T13:46:05Z

executor/src/witgen/jit/witgen_inference.rs

-                ..
-            }) => self.process_call(can_process, *id, multiplicity, &tuple.0, row_offset),
+            // TODO(bus_interaction)
+            Identity::PhantomBusInteraction(_) => ProcessResult::empty(),


Bug: Witgen filters out Bus interactions, so processing a call here leads to witgen not being able to find the called machine by the identity ID. This is also not trivial, because we'd probably want to use the receiving bus interaction.

Right now, all bus interactions also come with a phantom lookup / permutation, so they can safely be ignored. // TODO(bus_interaction) mark the places that need to be adjusted once this is removed as part of #2184.

georgwiese · 2024-12-30T13:46:32Z

executor/src/witgen/machines/block_machine.rs

@@ -445,7 +445,7 @@ impl<'a, T: FieldElement> BlockMachine<'a, T> {

                let updates = updates.report_side_effect();

-                let global_latch_row_index = self.data.len() - 1 - self.block_size + self.latch_row;
+                let global_latch_row_index = self.data.len() - self.block_size + self.latch_row;


Bug: This -1 is just wrong.

georgwiese · 2024-12-30T13:46:41Z

executor/src/witgen/machines/dynamic_machine.rs

-            let latch_row = self.data.len() - 1;
+            // The block we just added contains the first row of the next block,
+            // so the latch row is the second-to-last row.
+            let latch_row = self.data.len() - 2;


Bug: See comment.

georgwiese · 2024-12-30T13:47:53Z

linker/src/lib.rs

-                refs.path = SymbolPath::from_parts(
-                    once(Part::Named(namespace.clone())).chain(refs.path.clone().into_parts()),
-                );
+                if !refs.path.is_std() {


Bug: We used to namespace std namespaces as well. So, if there was a lookup from main into main_sub, using something in std::* in its expressions, it would turn into main::std::*.

georgwiese · 2024-12-30T13:49:20Z

pipeline/src/test_util.rs

+    test_mock_backend(pipeline_gl_native.clone());
+    run_pilcom_with_backend_variant(pipeline_gl_native, BackendVariant::Composite).unwrap();
+
+    let pipeline_gl_bus =


Note that this means we now run steps such as constant generation, witgen, etc twice, once for each linker mode.

georgwiese · 2024-12-30T13:52:21Z

pipeline/tests/asm.rs

@@ -127,6 +129,8 @@ fn empty() {
 }

 #[test]
+// TODO: https://github.com/powdr-labs/powdr/issues/2292


I don't think this is really worth fixing, see my comment in #2292. But in principle, we could track which namespace identities are generated in.

georgwiese · 2024-12-30T13:55:07Z

test_data/asm/single_operation.asm

+    col witness w;
+    w = w * w;


Somehow, without this, the columns generated by the bus interaction end up in the std::well_known namespace, which doesn't have a degree... I thought it's not worth fixing it, because a machine without a column is very unrealistic.

github-actions

⚠️ 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: `737bd78`	Previous: `2ad6c4f`	Ratio
`evaluator-benchmark/sqrt_879882356`	`36805` ns/iter (`± 23`)	`29318` ns/iter (`± 28`)	`1.26`
`evaluator-benchmark/sqrt_1882356`	`31520` ns/iter (`± 16`)	`24558` ns/iter (`± 16`)	`1.28`
`evaluator-benchmark/sqrt_1187956`	`31052` ns/iter (`± 13`)	`24467` ns/iter (`± 27`)	`1.27`
`evaluator-benchmark/sqrt_56`	`22249` ns/iter (`± 8`)	`17424` ns/iter (`± 9`)	`1.28`

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

This reverts commit 6b949ad.

georgwiese · 2024-12-30T14:23:00Z

Hmm, I reverted 6b949ad, because I'm getting a panic in the RISC-V executor. Guess that can be done in another PR.

georgwiese · 2024-12-30T16:31:54Z

pipeline/tests/asm.rs

@@ -263,9 +270,6 @@ fn dynamic_vadcop() {
    assert_eq!(witness_by_name["main_arith::y"].len(), 32);
    assert_eq!(witness_by_name["main_memory::m_addr"].len(), 32);

-    // Because machines have different lengths, this can only be proven
-    // with a composite proof.
-    run_pilcom_with_backend_variant(pipeline_gl.clone(), BackendVariant::Composite).unwrap();


Not sure if it is important to test PILCOM here? If we already have the mock prover? If yes, we'd have to make a new pipeline with LinkerMode::Native (which implies re-computing everything until the witness).

With a bus, PILCOM ends in some infinite loop. See this CI run.

Schaeff

Looks good to me

Enable Bus in tests

f4029f6

leonardoalt reviewed Dec 30, 2024

View reviewed changes

georgwiese added 7 commits December 30, 2024 11:54

JIT: Don't ignore bus interactions

5b87910

Linker: Don't namespace std paths

40b8009

Bug in for secondary VM multiplicity witgen

29e2f3a

Mark test as should_panic

8b20436

Clippy

0c0ec20

Bug in block machine multiplicity witgen

e6832b2

Use bus in more cases

737bd78

georgwiese commented Dec 30, 2024

View reviewed changes

github-actions bot reviewed Dec 30, 2024

View reviewed changes

georgwiese added 2 commits December 30, 2024 15:17

Enable bus in RISC-V tests

6b949ad

Revert "Enable bus in RISC-V tests"

97367b7

This reverts commit 6b949ad.

georgwiese marked this pull request as ready for review December 30, 2024 14:23

Remove PILCOM test

32ddf48

georgwiese commented Dec 30, 2024

View reviewed changes

Schaeff approved these changes Dec 30, 2024

View reviewed changes

georgwiese added this pull request to the merge queue Dec 30, 2024

Merged via the queue into main with commit 816e8c7 Dec 30, 2024
16 checks passed

georgwiese deleted the enable-bus branch December 30, 2024 18:20

georgwiese mentioned this pull request Jan 6, 2025

Change default linker to bus #2282

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable Bus in tests #2289

Enable Bus in tests #2289

georgwiese commented Dec 30, 2024 •

edited

Loading

leonardoalt Dec 30, 2024

georgwiese Dec 30, 2024

leonardoalt Dec 30, 2024

georgwiese Dec 30, 2024

georgwiese Dec 30, 2024

georgwiese Dec 30, 2024

georgwiese Dec 30, 2024

georgwiese Dec 30, 2024

georgwiese Dec 30, 2024

georgwiese Dec 30, 2024

github-actions bot left a comment

georgwiese commented Dec 30, 2024

georgwiese Dec 30, 2024

Schaeff left a comment

Enable Bus in tests #2289

Enable Bus in tests #2289

Conversation

georgwiese commented Dec 30, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot left a comment

Choose a reason for hiding this comment

⚠️ Performance Alert ⚠️

georgwiese commented Dec 30, 2024

Choose a reason for hiding this comment

Schaeff left a comment

Choose a reason for hiding this comment

georgwiese commented Dec 30, 2024 •

edited

Loading