Skip to content

AST-33: Word{8, 16, 32} <--> {Nat, Int} conversions#190

Merged
ggreif merged 38 commits intomasterfrom
gabor/conversions
Mar 5, 2019
Merged

AST-33: Word{8, 16, 32} <--> {Nat, Int} conversions#190
ggreif merged 38 commits intomasterfrom
gabor/conversions

Conversation

@ggreif
Copy link
Contributor

@ggreif ggreif commented Feb 26, 2019

A blueprint for the conversion semantics (Word32 in particular) is written down in https://dfinity.atlassian.net/browse/AST-63. The interpreter is now ready (with the exception of Word64 which is a bit tricky as it may be lossy when converting to Int/Nat in the absence of proper bignums).

I would have preferred to handle the conversions Word* -> {Nat,Int} without the shifts in prelude.ml, as it breaks abstraction, but could not find a suitable method that would do the shifting based on the representation (SubRep) for me. I'd welcome a tip how to funnel the shift amount through the module machinery (if this is possible at all; I honestly doubt it). What I was trying to do is to somehow bake it into MakeWord's to_bits method, but I have failed so far.

Compilation of many Word* arithmetics and bitwise primitives is lacking (test/run/words.as tracks this), I'd tackle this and the trivial refactoring in other PRs.

Below is the laundry list:

  • Nat -> Word32 only works up to 2**31-1 in the interpreter (compiler should be fine).
  • Nat -> Word32 does not check for proper bignums (in the compiler CG).
  • Executing the wasm binary causes an unreachable trap (missing (==) @Word32 codegen).
  • Word32 has underlying OCaml Int32, which is signed (thus reading is signed too).
  • Write story that iteration protocol should internally use BoxedSmallWord instead of BoxedInt.
  • A bunch of refactoring (Wasm.Value, unboxed_zero, unboxed_one, etc.).
  • Compilation of Word* arithmetics and bitwise primitives.
  • Word64.
  • Use awk to obtain a Haskell version of test/run/words.as, and run it, comparing outputs.

Any hints welcome.

@nomeata
Copy link
Contributor

nomeata commented Feb 27, 2019

I would prefer the interpreter, as it is our spec, to get things right from the start. The compiler may lag behind.

Maybe the interpreter should just use Value.Int everywhere, and then implement the correct bounds check/wraps in the corresponding operations? This way, we clearly document the desired semantics in the interpreter, without implicitly using whatever semantics int32 has on Ocaml.

@ggreif
Copy link
Contributor Author

ggreif commented Feb 27, 2019

@nomeata For the reference interpreter I'd even prefer nativeint, as it provides all the bit-fiddling ops, that I'll need for Word*.
The implementation needs to assert that nativeint is capable of holding a Word64.

I agree, that the implementation should strive for being the most clean possible. The semantics of the Word{8,16,32,64} types belong into Jira/Confluence. I'll begin with that soon. But should we give @matthewhammer a preview with what we have now (modulo tweaks)?

@nomeata
Copy link
Contributor

nomeata commented Feb 28, 2019

nice, printInt can print Nat too

Yes, that works simply by subtyping: Every x:Nat is also an x:Int.

ggreif added 2 commits March 1, 2019 13:55
* along with tagging, untagging
* updated conversion primitives

This is still work in progress, esp. naming
needs to be ironed out. Refactoring due later.
Copy link
Contributor

@nomeata nomeata left a comment

Choose a reason for hiding this comment

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

Looking good!

@ggreif ggreif changed the title AST-33: Word32 <--> Nat conversions AST-33: Word32 <--> {Nat, Int} conversions Mar 1, 2019
Copy link
Contributor Author

@ggreif ggreif left a comment

Choose a reason for hiding this comment

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

Naming is still a bit inconsistent, the small things should be i32, probably.

@nomeata
Copy link
Contributor

nomeata commented Mar 1, 2019

Are you reviewing your own code? :-)

@nomeata
Copy link
Contributor

nomeata commented Mar 1, 2019

(all comments reasonable, btw)

@ggreif ggreif changed the title AST-33: Word32 <--> {Nat, Int} conversions AST-33: Word{8, 16, 32} <--> {Nat, Int} conversions Mar 2, 2019
@ggreif ggreif marked this pull request as ready for review March 3, 2019 16:53
@ggreif ggreif requested a review from nomeata March 3, 2019 17:12
Copy link
Contributor

@nomeata nomeata left a comment

Choose a reason for hiding this comment

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

Sorry, I missed that this PR changed from Draft to ready. Looks good!

Small values (just <2^5 for now, so that both code paths are well-tested)
are stored unboxed, tagged, see BitTagged.

The heap layout of a BoxedWord is:
Copy link
Contributor

Choose a reason for hiding this comment

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

You write BoxedWord, but the module is still called BoxedInt.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll address this in the follow-up.

*)

let payload_field = Int32.add Tagged.header_size 0l
let payload_field = Tagged.header_size
Copy link
Contributor

Choose a reason for hiding this comment

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

That was actually intentional, to make it clear it’s the “0ths” field after the header. But maybe it’s too confusing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nobody looks at the expansion anyway...

@ggreif ggreif merged commit 8552673 into master Mar 5, 2019
@ggreif ggreif mentioned this pull request Mar 5, 2019
dfinity-bot added a commit that referenced this pull request Jun 23, 2023
## Changelog for ic-hs:
Branch: master
Commits: [dfinity/ic-hs@52369fa1...f092a2d5](dfinity/ic-hs@52369fa...f092a2d)

* [`12590345`](dfinity/ic-hs@1259034) bump cachix/install-nix-action ([dfinity/ic-hs⁠#201](https://github.com/dfinity/ic-hs/issues/201))
* [`2ef5690c`](dfinity/ic-hs@2ef5690) Bump openssl from 0.10.48 to 0.10.55 in /httpbin-rs ([dfinity/ic-hs⁠#202](https://github.com/dfinity/ic-hs/issues/202))
* [`46af8266`](dfinity/ic-hs@46af826) sync httpbin implementation with ic repo ([dfinity/ic-hs⁠#193](https://github.com/dfinity/ic-hs/issues/193))
* [`1fd276eb`](dfinity/ic-hs@1fd276e) implement Call context removal transition ([dfinity/ic-hs⁠#191](https://github.com/dfinity/ic-hs/issues/191))
* [`11c00d2a`](dfinity/ic-hs@11c00d2) catch HTTP exception from HTTPS outcalls ([dfinity/ic-hs⁠#195](https://github.com/dfinity/ic-hs/issues/195))
* [`9eb5bf48`](dfinity/ic-hs@9eb5bf4) deleted call contexts prevent canister from stopping (again) ([dfinity/ic-hs⁠#190](https://github.com/dfinity/ic-hs/issues/190))
* [`f092a2d5`](dfinity/ic-hs@f092a2d) implement and test canister history ([dfinity/ic-hs⁠#198](https://github.com/dfinity/ic-hs/issues/198))
mergify bot pushed a commit that referenced this pull request Jun 23, 2023
## Changelog for ic-hs:
Branch: master
Commits: [dfinity/ic-hs@52369fa1...f092a2d5](dfinity/ic-hs@52369fa...f092a2d)

* [`12590345`](dfinity/ic-hs@1259034) bump cachix/install-nix-action ([dfinity/ic-hs⁠#201](https://github.com/dfinity/ic-hs/issues/201))
* [`2ef5690c`](dfinity/ic-hs@2ef5690) Bump openssl from 0.10.48 to 0.10.55 in /httpbin-rs ([dfinity/ic-hs⁠#202](https://github.com/dfinity/ic-hs/issues/202))
* [`46af8266`](dfinity/ic-hs@46af826) sync httpbin implementation with ic repo ([dfinity/ic-hs⁠#193](https://github.com/dfinity/ic-hs/issues/193))
* [`1fd276eb`](dfinity/ic-hs@1fd276e) implement Call context removal transition ([dfinity/ic-hs⁠#191](https://github.com/dfinity/ic-hs/issues/191))
* [`11c00d2a`](dfinity/ic-hs@11c00d2) catch HTTP exception from HTTPS outcalls ([dfinity/ic-hs⁠#195](https://github.com/dfinity/ic-hs/issues/195))
* [`9eb5bf48`](dfinity/ic-hs@9eb5bf4) deleted call contexts prevent canister from stopping (again) ([dfinity/ic-hs⁠#190](https://github.com/dfinity/ic-hs/issues/190))
* [`f092a2d5`](dfinity/ic-hs@f092a2d) implement and test canister history ([dfinity/ic-hs⁠#198](https://github.com/dfinity/ic-hs/issues/198))
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