Skip to content

Opt const eval#17

Draft
Al-Kindi-0 wants to merge 11 commits into
mainfrom
opt_const_eval
Draft

Opt const eval#17
Al-Kindi-0 wants to merge 11 commits into
mainfrom
opt_const_eval

Conversation

@Al-Kindi-0
Copy link
Copy Markdown
Owner

For some reason the previous PR says that it has been merged. I am creating this new PR which implements the improvements suggested in the old one.
Sorry for the mess.

@Al-Kindi-0 Al-Kindi-0 requested a review from bobbinth November 2, 2022 15:54
Comment thread air/src/air/context.rs Outdated
Comment thread math/src/field/f64/mod.rs Outdated
Comment on lines +104 to +117
let mut b = self;

if power == 0 {
return Self::ONE;
} else if b == Self::ZERO {
return Self::ZERO;
}

let mut r = if power & 1 == 1 { b } else { Self::ONE };
for i in 1..64 - power.leading_zeros() {
b = b.square();
if (power >> i) & 1 == 1 {
r *= b;
}
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This is fine to have for testing purposes - but it probably shouldn't be a part of this PR.

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

Sure, I will remove it before making the PR against Novi/Winterfell main.

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

Actually, I will just change it back so that I don't forget it.

/// always $n - 1$. Thus, once we divide out the divisor, the evaluations will represent a
/// polynomial of degree $D$.
pub fn merge_evaluations<B, F>(&self, evaluations: &[F], x: B) -> E
pub fn merge_evaluations<B, F>(&self, evaluations: &[F], xp: B) -> E
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

The comment above probably should be updated.

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

Done, let me know what you think.

Comment thread prover/src/constraints/evaluator.rs Outdated
Comment thread prover/src/domain.rs
}

/// Returns (offset * g^(step))^degree_adjustment.
pub fn get_ce_x_power_at<A: Air<BaseField = B>>(
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

We could try adding #[inline(always)] attribute to this function (and maybe some other functions). Sometimes, this improves performance.

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

Sure, I did here and in the helper function on the same file.

Comment thread prover/src/domain.rs Outdated
Comment on lines +110 to +111
let index = index % (self.ce_domain_size());
let xp = self.ce_domain()[index] * *self.degree_adj_map().get(&degree_adjustment).unwrap();
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I wonder if instead of % we could use a shift here somehow (because we know that ce_domain_size is a power of 2).

Also, we should change unwrap to expect (we shouldn't use unwrap's in non-test code).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants