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

Optimization: replace sub vN, vN with zero #7187

Closed
asterite opened this issue Jan 24, 2025 · 0 comments · Fixed by #7189
Closed

Optimization: replace sub vN, vN with zero #7187

asterite opened this issue Jan 24, 2025 · 0 comments · Fixed by #7189

Comments

@asterite
Copy link
Collaborator

While looking at the SSA for this code:

use std::hash::sha256::sha256;

fn main(input: [u8; 32]) -> pub [u8; 32] {
    sha256(input)
}

I found this instruction:

    v172 = sub v41, v41

I think that could be replaced with the constant zero. There are two occurrences of that in the final SSA. Maybe it could also help with constant propagation, etc.

I tried implementing this with this patch:

diff --git a/compiler/noirc_evaluator/src/ssa/ir/instruction/binary.rs b/compiler/noirc_evaluator/src/ssa/ir/instruction/binary.rs
index df1e8f537d..14cddb7f41 100644
--- a/compiler/noirc_evaluator/src/ssa/ir/instruction/binary.rs
+++ b/compiler/noirc_evaluator/src/ssa/ir/instruction/binary.rs
@@ -103,6 +103,11 @@ impl Binary {
             );
         }

+        if self.lhs == self.rhs {
+            let zero = dfg.make_constant(FieldElement::zero(), lhs_type);
+            return SimplifyResult::SimplifiedTo(zero);
+        }
+
         let operator = if lhs_type == NumericType::NativeField {
             // Unchecked operations between fields or bools don't make sense, so we convert those to non-unchecked
             // to reduce noise and confusion in the generated SSA.

It works for a simple program like this one:

fn main(input: Field) -> pub Field {
    input - input
}

(it now returns Field 0 instead of doing the subtraction)

but the compiler enters a loop in the sha256 case and I don't understand why. It seems it happens during unrolling.

@github-project-automation github-project-automation bot moved this to 📋 Backlog in Noir Jan 24, 2025
@github-project-automation github-project-automation bot moved this from 📋 Backlog to ✅ Done in Noir Jan 30, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

1 participant