-
Notifications
You must be signed in to change notification settings - Fork 104
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
Conversation
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).
In terms of gas, no changes are observed in 3 tests. |
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
There was a problem hiding this 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.
Co-authored-by: Andreas Rossberg <[email protected]>
Co-authored-by: Claudio Russo <[email protected]>
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) |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
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) |
There was a problem hiding this comment.
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....
There was a problem hiding this comment.
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.
Co-authored-by: Claudio Russo <[email protected]>
Co-authored-by: Claudio Russo <[email protected]>
fcdf717
to
3a0ab52
Compare
* 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)
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 trappingconversions, and
num_warp_t1_2
is for wrapping (currently: to Word,and between fixed types). The actual
mo:prim
module stays compatible.