Add spark2

[jadnohra]

No description provided.

[jadnohra]

• Add beamish #864
• Add spark2 #851 👈 (View in Graphite)
• main

---

How to use the Graphite Merge Queue

Add the label merge-ready to this PR to add it to the merge queue.

You must have a Graphite account in order to use the merge queue. Sign up using this link.

An organization admin has enabled the Graphite Merge Queue in this repository.

Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue.

This stack of pull requests is managed by Graphite. Learn more about stacking.

[graphite-app]

There's a logic issue in the pattern matching condition. The current code:

if let Some(pattern_op) = op
    && pattern_op != expr_op {
        return false;
    }

This will always evaluate to false when pattern_op is Some because the && is binding too tightly with the comparison.

To fix this, either use a nested if statement:

if let Some(pattern_op) = op {
    if pattern_op != *expr_op {
        return false;
    }
}

Or use a match statement for clearer control flow:

match op {
    Some(pattern_op) if pattern_op != *expr_op => return false,
    _ => {}
}

Suggested change

	if let Some(pattern_op) = op
	&& pattern_op != expr_op {
	return false;
	}
	if let Some(pattern_op) = op {
	if pattern_op != expr_op {
	return false;
	}
	}

Spotted by Diamond



Is this helpful? React 👍 or 👎 to let us know.

[graphite-app]

Dependencies must use workspace-wide declarations. Replace binius-core = { path = "../core" } with binius-core.workspace = true and binius-field = { path = "../field" } with binius-field.workspace = true. These dependencies must be declared in the workspace root Cargo.toml file.

Spotted by Diamond (based on custom rule: Irreducible Rust and Cargo)



Is this helpful? React 👍 or 👎 to let us know.

[graphite-app]

The Unary case should handle both scenarios: when the inner expression changes and when it doesn't. Currently, it only returns Some when the inner expression changes, unlike the Binary case which correctly handles both scenarios. Consider updating to:

Expr::Unary { op, expr: inner } => {
    let new_inner = self.apply_rule_recursive(rule, inner)
        .map(Box::new)
        .unwrap_or_else(|| inner.clone());
    
    if &new_inner != inner {
        Some(Expr::Unary {
            op: *op,
            expr: new_inner,
        })
    } else {
        None
    }
}

This ensures consistent behavior with the other expression types.

Suggested change

	Expr::Unary { op, expr: inner } => {
	self.apply_rule_recursive(rule, inner).map(|new_inner| {
	Expr::Unary {
	op: *op,
	expr: Box::new(new_inner),
	}
	})
	}
	Expr::Unary { op, expr: inner } => {
	let new_inner = self.apply_rule_recursive(rule, inner)
	.map(Box::new)
	.unwrap_or_else(|| inner.clone());
	if &new_inner != inner {
	Some(Expr::Unary {
	op: *op,
	expr: new_inner,
	})
	} else {
	None
	}
	}

Spotted by Diamond



Is this helpful? React 👍 or 👎 to let us know.

[graphite-app]

The pattern uses hardcoded rotation values of 0 in UnOp::Ror(0), but the comment suggests it should match any rotation amount. Consider either:

1. Using None for the op field to match any rotation operation:

Pattern::Unary {
    op: None,
    pattern: Box::new(Pattern::var("x")),
}

2. Or creating specific patterns for the exact rotation combinations used in SHA256 (e.g., Sigma0: 2, 13, 22 and Sigma1: 6, 11, 25).

This would ensure the pattern correctly matches the intended SHA256 rotation operations.

Suggested change

	Pattern::xor_chain(vec![
	Pattern::Unary {
	op: Some(UnOp::Ror(0)), // Will be matched with actual rotation amounts
	pattern: Box::new(Pattern::var("x")),
	},
	Pattern::Unary {
	op: Some(UnOp::Ror(0)),
	pattern: Box::new(Pattern::var("x")),
	},
	Pattern::Unary {
	op: Some(UnOp::Ror(0)),
	pattern: Box::new(Pattern::var("x")),
	},
	])
	Pattern::xor_chain(vec![
	Pattern::Unary {
	op: None, // Will match any rotation operation
	pattern: Box::new(Pattern::var("x")),
	},
	Pattern::Unary {
	op: None,
	pattern: Box::new(Pattern::var("x")),
	},
	Pattern::Unary {
	op: None,
	pattern: Box::new(Pattern::var("x")),
	},
	])

Spotted by Diamond



Is this helpful? React 👍 or 👎 to let us know.