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

Value.Numerics: Refactoring to prepare word deprecation #2324

Merged
merged 12 commits into from
Feb 10, 2021

Conversation

nomeata
Copy link
Collaborator

@nomeata nomeata commented Feb 9, 2021

this extracts, off #2309, the refactoring of value, without user-visible changes.

This should make reviewing #2309 easier, and also pave the way to
deprecating the word type.

This already includes the implementation of bitoperations and shifting operations
in value_mo/numerics.ml, but does ont yet allow their use in Motoko.

Because this is based off #2309, it includes some changes to the prims.
In particular, the num_conv_t1_t2 family is now for trapping
conversions, and num_warp_t1_2 is for wrapping (currently: to Word,
and between fixed types). The actual mo:prim module stays compatible.

this extracts, off #2309, the refactoring of `value`, without any
user-visible changes.

This should make reviewing #2309 easier, and also pave the way to
deprecating the word type.

Because this is based off #2309, it includes some changes to the prims.
In particular, the `num_conv_t1_t2` family is now for trapping
conversions, and `num_warp_t1_2` is for wrapping (currently: to Word,
and between fixed types).
@dfinity-ci
Copy link

In terms of gas, no changes are observed in 3 tests.
In terms of size, no changes are observed in 3 tests.

@nomeata nomeata marked this pull request as ready for review February 9, 2021 11:53
@nomeata nomeata requested a review from crusso February 9, 2021 12:08
nomeata added a commit that referenced this pull request Feb 9, 2021
Builds on top of #2324, and is extracted from #2324.

Because this should be backwards-compatible, there is still the +>>
operator. Once `WordN` is removed, we only want and need the `>>`
operator (taking the signedness from the type). Until then, `

 * `>>` is unsigned on Nat and Word, and signed on Int.
 * `+>>` is always signed
Copy link
Contributor

@rossberg rossberg left a comment

Choose a reason for hiding this comment

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

Frontend / operator stuff LGTM.

src/mo_values/numerics.mli Show resolved Hide resolved
src/mo_values/operator.ml Outdated Show resolved Hide resolved
src/mo_values/prim.ml Outdated Show resolved Hide resolved
Co-authored-by: Andreas Rossberg <[email protected]>
src/mo_values/numerics.ml Outdated Show resolved Hide resolved
nomeata and others added 3 commits February 9, 2021 15:44
and some knock-on effects for `charToWord32`
| Nat32Lit n -> Const.Word32 (Int32.of_int (Value.Nat32.to_int n))
| Int64Lit n -> Const.Word64 (Big_int.int64_of_big_int (Value.Int_64.to_big_int n))
| Nat64Lit n -> Const.Word64 (Big_int.int64_of_big_int (nat64_to_int64 n))
| NatLit n -> Const.BigInt (Numerics.Nat.to_big_int n)
Copy link
Contributor

Choose a reason for hiding this comment

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

I assume the changes in the word cases are for uniformity?

Copy link
Collaborator Author

@nomeata nomeata Feb 9, 2021

Choose a reason for hiding this comment

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

more for historical reasons. In #2309 I removed word types alltogether. Then I learened that this was premature mumble mumble, and I added the word types to that PR, based on what is there (as if it never existed).

And then, I extracted part of those changes into this PR. In that sense, the diff is not minimal to where we come from (master), but to where we want to got (#2309)

@@ -7127,12 +7122,12 @@ and compile_exp (env : E.t) ae exp =
SR.UnboxedWord64,
compile_exp_as env ae SR.UnboxedWord64 e ^^
G.i (Unary (Wasm.Values.I64 I64Op.Popcnt))
| OtherPrim "clz8", [e] -> SR.Vanilla, compile_exp_vanilla env ae e ^^ TaggedSmallWord.clz_kernel Type.Word8
| OtherPrim "clz16", [e] -> SR.Vanilla, compile_exp_vanilla env ae e ^^ TaggedSmallWord.clz_kernel Type.Word16
| OtherPrim "clz8", [e] -> SR.Vanilla, compile_exp_vanilla env ae e ^^ TaggedSmallWord.clz_kernel Type.Nat8
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it ok that these use Nats but prelude types are still Word based? I assume so but just pointing out the change.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, this is a little code smell: TaggedSmallWord takes a (Motoko-level) type argument, but all it really cares about is the bit width. This just anticipates removing the Word type.

I was even considering refactoring our types to use Fixed of (Signedness * Bitwidth)… it would simplify a bunch of code like this.

Copy link
Contributor

@crusso crusso left a comment

Choose a reason for hiding this comment

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

LGTM

src/lowering/desugar.ml Outdated Show resolved Hide resolved
src/lowering/desugar.ml Outdated Show resolved Hide resolved
let twoRaised63 = power_int_positive_int 2 63 in
let q, r = quomod_big_int (Value.Nat64.to_big_int n) twoRaised63 in
if sign_big_int q = 0 then r else sub_big_int r twoRaised63
if ge_big_int n (power_int_positive_int 2 63)
Copy link
Contributor

Choose a reason for hiding this comment

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

I'll trust you that these are computing the same result....

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That’s good, I just hope they do :-)

But with the spec in the comment (in particular the restriction on the domain), I found this code easier to understand.

nomeata and others added 2 commits February 10, 2021 09:41
@nomeata nomeata added the automerge-squash When ready, merge (using squash) label Feb 10, 2021
@nomeata nomeata force-pushed the joachim/value-refactor branch from fcdf717 to 3a0ab52 Compare February 10, 2021 08:48
@mergify mergify bot merged commit b62a067 into master Feb 10, 2021
@mergify mergify bot removed the automerge-squash When ready, merge (using squash) label Feb 10, 2021
@nomeata nomeata deleted the joachim/value-refactor branch February 10, 2021 08:56
mergify bot pushed a commit that referenced this pull request Feb 12, 2021
* The bit-operations are added to Nat8, Int8 etc.
   On these, we only have `>>` (the signeness of the shift is derived from the type), no `+>>`.
   That operator is only available on `WordN`, and will be removed with it.

* Wrapping operators `+%`, `-%`, `*%` and `**%` are added.
  `*%` traps on negative exponent for `IntN`.

  The assignment variants (`+%=`, `-%=`, `+%=`, `**%=`) are added as well
  
* We now have two conversions `Nat→Nat8`, a trapping and a wrapping one, and likewise for `Int→Int8`.
  This builds on the bifurcaton of prims in #2324

* Conversions between equal bit-width numbers are always wrapping, and exists for all of them, including
   directly from `NatN` to `IntN` and back.

* Little testing in this PR, but #2327 (which is based on this one) does some thorough randomized checking.
  The test suite is ported to not use `WordN` in #2309 and also passes.

* A PR against motoko-base to expose the new functionality is in dfinity/motoko-base#217.
  (But this PR can go in before, it is compatible with the previous base.)

* A changelog entry is added

* The user’s guide is updated (picking changes from #2309 as appropriate, but not removing mention of `Word` entirely)
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