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 SignedWadMath edge case check #381

Merged
merged 2 commits into from
Aug 8, 2023

Conversation

Philogy
Copy link
Contributor

@Philogy Philogy commented Aug 8, 2023

Description

I've changed the SignedWadMul.wadMul to do the overflow & edge case check in one if:

if iszero(
    and(
        or(iszero(x), eq(sdiv(r, x), y)),
        or(lt(x, not(0)), sgt(y, 0x8000000000000000000000000000000000000000000000000000000000000000))
    )
) {
    revert(0, 0)
}

This saves gas at runtime for all cases except the edge case because the resulting bytecode only uses 1 JUMPI vs. 2 to check the conditions. Branches are currently quite expensive making it worth the slightly higher cost of combining multiple expressions.

To save further gas I've transformed the !(x == -1 && y == type(int256).min) check into x != -1 || y != type(int256).min to minimize the use of ISZERO. The inequality checks both need to have binary results so the resulting bitwise expression and(no_overflow, or(x_not_neg1, y_not_min_signed)) equates the logical expression across the whole set of values. To achieve this LT and SGT are abused as != operators, this is possible here as -1 represents the highest possible unsigned integer and 0x8000000000000000000000000000000000000000000000000000000000000000 the lowest signed number.

The final inline-assembly expression represents:

require(
    (x == 0 || (r / x == y))
    && (x != -1 || y != type(int256).min)
);

Checklist

  • Ran forge snapshot?
  • Ran npm run lint?
  • Ran forge test?

Pull requests with an incomplete checklist will be thrown out.

@transmissions11
Copy link
Owner

nice, lgtm!

@transmissions11 transmissions11 merged commit fadb2e2 into transmissions11:main Aug 8, 2023
1 of 2 checks passed
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