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

Optimize rpow(x,n,base) #78

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Optimize rpow(x,n,base) #78

wants to merge 2 commits into from

Conversation

k06a
Copy link
Contributor

@k06a k06a commented Oct 19, 2019

  1. Optimize rpow(x,n,base) for the case x != 0 && n == 0 (291 gas vs 383 gas)

    switch x case 0 {switch n case 0 {z := b} default {z := 0}}
    default {
       ...
    }

    =>

    switch n case 0 { z := b }
    default {
        switch x case 0 { z := 0 }
        default {
            ...
        }
    }
  2. Simplify overflow check for squaring in rpow(x,n,base) (2355 gas vs 2388 gas for x = 2, n = 255)

    let xx := mul(x, x)
    if iszero(eq(div(xx, x), x)) { revert(0,0) }

    =>

    let xx := mul(x, x)
    if shr(128, x) { revert(0,0) }

@k06a k06a changed the title Optimize rpow(x,n,base) for the case x != 0 && n == 0 (291 gas vs 383 gas) Optimize rpow(x,n,base) Oct 19, 2019
@k06a k06a force-pushed the optimize/rpow branch 3 times, most recently from 8724783 to 7061ea9 Compare October 19, 2019 21:36
@livnev
Copy link
Contributor

livnev commented Oct 20, 2019

@kmbarry1 can you get these optimisations to pass the spec? 😛

@kmbarry1
Copy link
Contributor

I will give it a try today!

@kmbarry1 kmbarry1 requested review from kmbarry1 and a team October 21, 2019 16:06
@kmbarry1
Copy link
Contributor

Also, just to give "official" comment on this: looks good, but there are two things I want to check prior to merging:

  1. gas for x != 0 && n != 0 is not worsened (I don't think it is, looks like it should be at least the same, maybe better, but want to check explicitly)
  2. must successfully formally verify this

@gbalabasquer
Copy link
Contributor

@kmbarry1 do we have any update on this regarding FV?

@kmbarry1
Copy link
Contributor

Am getting through the backlog of changes that are definitely going in before analyzing this.

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.

4 participants