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

Fixed point types. #11019

Closed
wants to merge 3 commits into from
Closed

Fixed point types. #11019

wants to merge 3 commits into from

Conversation

chriseth
Copy link
Contributor

Closes #409

@ekpyron
Copy link
Member

ekpyron commented Mar 1, 2021

I wonder whether it'd be wise to decide up front if we want to never support 256-bit multiplication on these or how to deal with that wrt overflows - having multiplication for 256-bit ones behave differently from all others might be rather confusing... (or maybe not, not entirely sure...)

@chriseth
Copy link
Contributor Author

chriseth commented Mar 3, 2021

@ekpyron why would you say multiplication behaves differently? Currently we have "arithmetic operations are performed in two's complement / modulo 2**bits". For uint64 for example, the actual computation is done with higher bit width and then truncated. This would be the same for fixed256x80. Essentially for all types and operators, we perform the operation in arbitrary precision arithmetics and then truncate.

@ekpyron
Copy link
Member

ekpyron commented Mar 3, 2021

@ekpyron why would you say multiplication behaves differently? Currently we have "arithmetic operations are performed in two's complement / modulo 2**bits". For uint64 for example, the actual computation is done with higher bit width and then truncated. This would be the same for fixed256x80. Essentially for all types and operators, we perform the operation in arbitrary precision arithmetics and then truncate.

And how would you do that? I'm not saying it has to behave differently, but that it's worth a thought what it would look like for the 256 bit type before defining that we do things in arbitrary precision only to later notice that that's too much hassle for 256-bits.

@chriseth
Copy link
Contributor Author

chriseth commented Mar 3, 2021

We have a "larger than word size" multiplication algorithm in the wasm transform. I would do it in a similar way.

@ekpyron
Copy link
Member

ekpyron commented Mar 3, 2021

We have a "larger than word size" multiplication algorithm in the wasm transform. I would do it in a similar way.

Ok, fair enough. And we're sure that's the way to go? I mean it would mean that the 128-bit version is cheaper, that's something we'd probably need to communicate, because people might assume that the 256-bit version is the least hassle (and I recall that people don't read documentation ;-)). But yeah, I'm just wondering, I'm fine with going that way! The alternatives to always revert on intermediate value overflows or to have only the 256-bit version do that are both probably significantly worse, so probably the best option regardless.

@ekpyron
Copy link
Member

ekpyron commented Mar 3, 2021

I mean of course just not having a 256-bit version could be a viable and maybe even quite reasonable version actually...

@chriseth
Copy link
Contributor Author

chriseth commented Mar 3, 2021

fixed will be an alias for fixed128x80 correction: fixed128x18 - 128 bits overall and 18 decimal places after the decimal point.

@axic
Copy link
Member

axic commented Apr 23, 2021

I've looked into reviving this and I think we may be better off splitting this into multiple steps/PRs:

  • semantic tests (and codegen fixes) for all kinds of assignment and input/output (this also needs isoltest improvements)
  • ... for constant expressions
  • ... for fixedpoint arrays
  • ... for conversions
  • ... for arithmethic

Pushed some tests and the result is:

  • state_variables: Fixed point types not implemented.
  • variables: Line 11: Unexpected character: '.' (isoltest missing support
  • arrays: Error: Contract or array type expected.
  • constants: Fixed point types not implemented.

}
// ----
// f() ->
// g(fixed128x80,fixed128x80): 9.871, 88888888.0 ->
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Suggested change
// g(fixed128x80,fixed128x80): 9.871, 88888888.0 ->
// g(fixed128x18,fixed128x18): 9.871, 88888888.0 ->

@chriseth
Copy link
Contributor Author

chriseth commented Aug 3, 2021

Replaced by several independent PRs. The codegen for operators is not in the other PRs but it is easy to recreate.

@chriseth chriseth closed this Aug 3, 2021
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.

Fixed point types
3 participants