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

Add U128 type #1600

Merged
merged 39 commits into from
May 25, 2022
Merged

Add U128 type #1600

merged 39 commits into from
May 25, 2022

Conversation

adlerjohn
Copy link
Contributor

@adlerjohn adlerjohn commented May 18, 2022

Fixes #1478

Blocked by

Soft-blocked by FuelLabs/fuel-tx#129, until which tests will quickly run out of gas. The current limit of 1M gas is hit after 4 multiplications or 2 divisions.

Implements https://en.wikipedia.org/wiki/Division_algorithm#Integer_division_(unsigned)_with_remainder for division.

@adlerjohn adlerjohn added enhancement New feature or request lib: std Standard library labels May 18, 2022
@adlerjohn adlerjohn self-assigned this May 18, 2022
@adlerjohn adlerjohn added the P: critical Should be looked at before anything else label May 19, 2022
sway-lib-std/src/u128.sw Outdated Show resolved Hide resolved
@adlerjohn adlerjohn marked this pull request as ready for review May 25, 2022 04:33
sway-lib-std/src/u128.sw Outdated Show resolved Hide resolved
sway-lib-std/src/u128.sw Outdated Show resolved Hide resolved
sway-lib-std/src/u128.sw Outdated Show resolved Hide resolved
sway-lib-std/src/u128.sw Show resolved Hide resolved
sway-lib-std/src/u128.sw Outdated Show resolved Hide resolved
sway-lib-std/src/u128.sw Outdated Show resolved Hide resolved
sway-lib-std/src/u128.sw Show resolved Hide resolved
sway-lib-std/src/u128.sw Show resolved Hide resolved
test/src/e2e_vm_tests/mod.rs Show resolved Hide resolved
Comment on lines 52 to 64
fn disable_overflow() {
asm(r1) {
movi r1 i3;
flag r1;
}
}

fn enable_overflow() {
asm(r1) {
movi r1 i0;
flag r1;
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Do these belong somewhere more general as pub definitions? Like num or somewhere?

Copy link
Contributor

@nfurfaro nfurfaro May 25, 2022

Choose a reason for hiding this comment

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

hmm. My understanding was that the flag op had to be used in the same asm block as the op you're trying to allow overflow for, otherwise, the bit is unset after the block returns. Maybe a misunderstanding on my part, perhaps I was confused with $of being cleared after an operation.
Is this working?

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, perhaps rather than movi r1 i3; here you could initialize a second register with the value as a named variable (or even better, as a const) to:
a.) help document the function better, and
b.) to follow the general guideline of "if it can be done outside of asm, do it outside of asm".

ie:

const WRAPPING_FLAG_VALUE = 3;
asm(r1: WRAPPING_FLAG_VALUE) {
 flag r1;
}

Copy link
Contributor

@nfurfaro nfurfaro May 25, 2022

Choose a reason for hiding this comment

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

And finally, it seems that the flag values for enable and disable are reversed.
An unset flag (ie: 0) should be the default state in which panic occurs on overflow.
Setting to a non-zero value should enable overflow.
Edit: I think it's actually just the names that seem reversed to me. like, disable_overflow should be disable_panic_on_overflow, and enable_overflow should be enable_panic_on_overflow.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, perhaps rather than movi r1 i3; here you could initialize a second register with the value as a named variable (or even better, as a const) to: a.) help document the function better, and b.) to follow the general guideline of "if it can be done outside of asm, do it outside of asm".

ie:

const WRAPPING_FLAG_VALUE = 3;
asm(r1: WRAPPING_FLAG_VALUE) {
 flag r1;
}

What about

asm(WRAPPING_FLAG_VALUE: 3) {
    flag WRAPPING_FLAG_VALUE;
}

Copy link
Contributor

Choose a reason for hiding this comment

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

I think a combination of a descriptively-named register and a const value would be best, and in line with tx.sw and other modules where we use named const values to initialize registers with.

Copy link
Contributor

@nfurfaro nfurfaro May 25, 2022

Choose a reason for hiding this comment

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

Do these belong somewhere more general as pub definitions? Like num or somewhere?

We had talked about a stdlib module for wrapping VM opcodes, this would probably fit the bill. Will look for the open issue.
#1074
Perhaps I'll just add a task to that issue to create a new module and extract these in a separate PR.
Done.
cc @otrho

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed in 36f5ef7. Filed #1664 to follow-up on cleaner constants.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@otrho given that no other part of the stdlib needs these functions, can we defer it to a future PR?

sway-lib-std/src/u128.sw Outdated Show resolved Hide resolved
sway-lib-std/src/u128.sw Show resolved Hide resolved
@adlerjohn adlerjohn requested review from Braqzen and otrho May 25, 2022 13:51
Braqzen
Braqzen previously approved these changes May 25, 2022
sway-lib-std/src/u128.sw Outdated Show resolved Hide resolved
Copy link
Contributor

@Braqzen Braqzen left a comment

Choose a reason for hiding this comment

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

Tuturu

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request lib: std Standard library P: critical Should be looked at before anything else
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Add new type U128
5 participants