Propagate known. #2325

chriseth · 2025-01-10T20:58:02Z

No description provided.

Co-authored-by: Georg Wiese <[email protected]>

…r into single_step_simple

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: `a3dc813`	Previous: `b961239`	Ratio
`jit-witgen-benchmark/jit_witgen_benchmark`	`51511629019` ns/iter (`± 46751484`)	`32073282431` ns/iter (`± 76394754`)	`1.61`
`executor-benchmark/keccak`	`437809691309` ns/iter (`± 3274197576`)	`16140744556` ns/iter (`± 112427593`)	`27.12`

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

…rence.

…ntities

chriseth · 2025-01-14T17:48:07Z

Error solving equality constraint:
  (main_arith::is_affine + main_arith::is_ec_add + main_arith::is_ec_double)
* (
  main_arith::do_mstore
    - (main_arith::CLK32[0] + main_arith::CLK32[1] + main_arith::CLK32[2] + main_arith::CLK32[3]
    + main_arith::CLK32[4] + main_arith::CLK32[5] + main_arith::CLK32[6] + main_arith::CLK32[7]
    + (main_arith::CLK32[8] + main_arith::CLK32[9] + main_arith::CLK32[10] + main_arith::CLK32[11]
    + main_arith::CLK32[12] + main_arith::CLK32[13] + main_arith::CLK32[14] + main_arith::CLK32[15]
    )))= 0
    on row 8:
    Linear constraint is not satisfiable: -(1 + main_arith::is_ec_double[8]) != 0

In the last line, the fact that it did not replace main_arith::is_ec_double[8] by an actual value means that it is known (i.e. the code contains a way to assign it) but only symbolically, and there are range constraints known on it such that the range constraint +1 excludes zero.

It probably also means that is_affine and is_ec_add are both known at compile time, one of them is zero, the other one is 1.

It also branched, but not sure if those are still active:

[DEBUG powdr_executor::witgen::jit::processor] Branching on variable main_arith::is_affine[0] with range [0, 1] & 0x1 at depth 0
[DEBUG powdr_executor::witgen::jit::processor] Branching on variable main_arith::is_mod[0] with range [0, 1] & 0x1 at depth 1

Co-authored-by: Georg Wiese <[email protected]>

…ntities

chriseth · 2025-01-16T09:42:40Z

executor/src/witgen/jit/processor.rs

-            });
+            let mut progress = false;
+            for (id, row_offset) in &self.identities {
+                if witgen.process_identity(can_process.clone(), id, *row_offset)? {


We need to use a for loop here because of the ? operator.

chriseth · 2025-01-16T09:44:29Z

executor/src/witgen/jit/single_step_processor.rs

@@ -237,7 +233,7 @@ if (VM::instr_add[0] == 1) {
        col fixed INSTR_ADD = [0, 1] + [0]*;
        col fixed INSTR_MUL = [1, 0] + [1]*;

-        pc' = pc + 1;


This change was needed because otherwise, the system is not solvable. Do a case analysis of the allowed values for pc and pc' and you will see why.

chriseth · 2025-01-16T09:45:55Z

executor/src/witgen/jit/witgen_inference.rs

@@ -186,7 +186,7 @@ impl<'a, T: FieldElement, FixedEval: FixedEvaluator<T>> WitgenInference<'a, T, F
            row_offset,
            rhs: VariableOrValue::Value(value),
        });
-        self.process_assignments();
+        self.process_assignments().unwrap();


This should not lead to a conflict. If that happens, we can propagate the error, but this function is called in some places where it is difficult to propagate the error.

chriseth · 2025-01-16T09:46:01Z

executor/src/witgen/jit/witgen_inference.rs

@@ -203,24 +203,71 @@ impl<'a, T: FieldElement, FixedEval: FixedEvaluator<T>> WitgenInference<'a, T, F
            row_offset,
            rhs: VariableOrValue::Variable(variable),
        });
-        self.process_assignments();
+        self.process_assignments().unwrap();


chriseth · 2025-01-16T09:46:57Z

executor/src/witgen/jit/witgen_inference.rs

@@ -255,11 +302,6 @@ impl<'a, T: FieldElement, FixedEval: FixedEvaluator<T>> WitgenInference<'a, T, F
        let Some(new_range_constraints) =
            can_process_call.can_process_call_fully(lookup_id, &known, &range_constraints)
        else {
-            log::trace!(


This was very noisy and not really helpful.

chriseth · 2025-01-16T09:47:50Z

executor/src/witgen/jit/witgen_inference.rs

@@ -363,7 +406,7 @@ impl<'a, T: FieldElement, FixedEval: FixedEvaluator<T>> WitgenInference<'a, T, F
            }
        }
        if progress {
-            self.process_assignments();
+            self.process_assignments().unwrap();


Again, too lazy to propagate the error here. Let's see if it happens.

chriseth and others added 10 commits December 20, 2024 11:36

Single step processor.

cce4375

Negative test.

c54c0dd

Merge remote-tracking branch 'origin/main' into single_step_simple

a40a133

Apply suggestions from code review

eb245ac

Co-authored-by: Georg Wiese <[email protected]>

Merge branch 'single_step_simple' of ssh://github.com/powdr-labs/powd…

1530cae

…r into single_step_simple

adjust tests.

1588eb2

Remove test function.

cc70c19

Merge branch 'remove_test_function' into single_step_simple

b13df2d

Fix testing functions.

96d5506

Merge branch 'single_step_simple'

4fc4a2e

github-actions bot reviewed Jan 10, 2025

View reviewed changes

chriseth force-pushed the propagate_known branch from c485e72 to 78bb381 Compare January 13, 2025 15:02

chriseth added 5 commits January 13, 2025 17:59

Provide completed identities and store completed inside witgen_infe…

64de47c

…rence.

fix

f44e52b

Merge remote-tracking branch 'origin/main' into provide_completed_ide…

d8e7273

…ntities

Simplify process summary.

f5c1f74

Cleanup

2945d10

chriseth force-pushed the propagate_known branch from 5932397 to 7770480 Compare January 14, 2025 10:49

chriseth changed the base branch from main to provide_completed_identities January 14, 2025 10:49

chriseth marked this pull request as ready for review January 14, 2025 14:24

Apply suggestions from code review

5423bb0

Co-authored-by: Georg Wiese <[email protected]>

chriseth force-pushed the propagate_known branch from 16de5ab to 2d9ce28 Compare January 15, 2025 10:22

chriseth added 7 commits January 15, 2025 10:25

fix expectations

64b622a

fix comment.

aedb50d

Merge remote-tracking branch 'origin/main' into provide_completed_ide…

4477ea7

…ntities

Propagate known

1b1129b

Fix test.

917f677

cleanup

1405bc0

Refactor.

f937252

chriseth added 7 commits January 15, 2025 10:31

Better errors.

73c9d81

Also print code generated so far.

6ce7f65

Compile-time check for covered offset.

cafd532

Consider conflicting branches.

5c9f1a5

Check assignments.

926c0ed

Better logging.

80c376f

Use BTreeSet for assignments.

c90b819

chriseth force-pushed the propagate_known branch from 2d9ce28 to c90b819 Compare January 15, 2025 10:32

fmt

0972be0

Base automatically changed from provide_completed_identities to main January 15, 2025 20:00

chriseth added 4 commits January 16, 2025 08:17

Merge branch 'provide_completed_identities'

052216c

Merge branch 'main' into propagate_known

4038131

Merge remote-tracking branch 'origin/main' into propagate_known

08e897a

fix

a3dc813

chriseth commented Jan 16, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Propagate known. #2325

Propagate known. #2325

chriseth commented Jan 10, 2025

github-actions bot left a comment •

edited

Loading

chriseth commented Jan 14, 2025 •

edited

Loading

chriseth Jan 16, 2025

chriseth Jan 16, 2025

chriseth Jan 16, 2025

chriseth Jan 16, 2025

chriseth Jan 16, 2025

chriseth Jan 16, 2025

Propagate known. #2325

Are you sure you want to change the base?

Propagate known. #2325

Conversation

chriseth commented Jan 10, 2025

github-actions bot left a comment • edited Loading

Choose a reason for hiding this comment

⚠️ Performance Alert ⚠️

chriseth commented Jan 14, 2025 • edited Loading

chriseth Jan 16, 2025

Choose a reason for hiding this comment

chriseth Jan 16, 2025

Choose a reason for hiding this comment

chriseth Jan 16, 2025

Choose a reason for hiding this comment

chriseth Jan 16, 2025

Choose a reason for hiding this comment

chriseth Jan 16, 2025

Choose a reason for hiding this comment

chriseth Jan 16, 2025

Choose a reason for hiding this comment

github-actions bot left a comment •

edited

Loading

chriseth commented Jan 14, 2025 •

edited

Loading