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

AST-33: Codegen for Word8/16/32 operations #216

Merged
merged 53 commits into from
Mar 12, 2019
Merged

AST-33: Codegen for Word8/16/32 operations #216

merged 53 commits into from
Mar 12, 2019

Conversation

ggreif
Copy link
Contributor

@ggreif ggreif commented Mar 5, 2019

Stories:

Probably nothing interesting for Word32. OTOH, Word8 and Word16 are kept in an MSB-aligned format with Vanilla SR. Carrying operations (add, sub, mul) are correctly working by discarding the ripple. Most logical ops are also not infectious, i.e. won't introduce nonzero bits in the lower part. Thus, the frequent operations come for free.
Similarly Char can be represented 8-bit-left-shifted as Vanilla.

This is the theory, and now also practice, as the tests pass :-)

I managed to keep the generating code mostly uniform, due to the additional bookkeeping being in helpers, that nop when nothing is to ne done.

I have also (kind-of) implemented a C++-like type based templating mechanism, see name_of_type.

Copy link
Collaborator

@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.

So, this means that 0xAB : Word8 is stored as 0xAB000000 : i32, right?

This does not fly yet: The (current) specification for Vanilla is that it is a pointer if the last bit is 0, and a scalar otherwise. So it would have to be 0xAB000001 : i32. You can probably trigger a bug this way if you send a message with your new data types.

(In fact, please do extend test/run-dfinity/data-params.as as you implement new data types).

The two solutions are:

  • Add the tag bit there (and complicate some of the operations), or
  • Swap the convention: Pointers are tagged and scalars are not. Andreas originally envisioned this, by representing a pointer to location x as x-1. We’d have to adjust where we call the system API (data.externalize); Load and Store can use the offset to fix that.

If you think we should do the latter, then that should happen on a separate branch first, and well tested, I think. Maybe let’s chat on Slack about it.

nomeata added a commit that referenced this pull request Mar 6, 2019
to make life easier for arithmetic on Word8/Word16 (#216)

(This is with GC disabled.)
@ggreif
Copy link
Contributor Author

ggreif commented Mar 6, 2019

Blocked on #217.

@ggreif
Copy link
Contributor Author

ggreif commented Mar 8, 2019

@nomeata I'd like to make popcnt type-polymorphic with specialised implementations (à la Haskell typeclass). Restricted for Word{8,16,32,64}.

Is there an example for such a thing? You wrote some show code back then...

ggreif and others added 9 commits March 10, 2019 12:20
this is not semantics changing, but may reduce the recursion depth
no need to sanitise when we know that the bit is clear
24a55ce Revert "another attempt to go green"
dc5e8bc Revert "try go green"
@ggreif
Copy link
Contributor Author

ggreif commented Mar 10, 2019

@nomeata I think this is ready for review now. The last bullet point (bounds check on word32ToChar) can wait, but I can also easily press it in, if so desired. The other bullet points I'd like to resolve in another PR.

into new module UnboxedSmallWord
Copy link
Collaborator

@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.

Ups, forgot to submit this comment.

src/compile.ml Show resolved Hide resolved
src/compile.ml Outdated Show resolved Hide resolved
src/compile.ml Show resolved Hide resolved
@ggreif ggreif self-assigned this Mar 11, 2019
fix serialisation of Tagged.SmallWord

this was a carry-over from before the pointer tagging was changed
instead of memcpy_words_skewed

this version is probably more efficient (less calls)
``` asm
          i32.const 1
          call $alloc_words
          tee_local $x'
          i32.const 11
          i32.store offset=1
          get_local $x'
          get_local $x
          i32.load offset=5
          i32.store offset=5
          get_local $x'
```
@ggreif
Copy link
Contributor Author

ggreif commented Mar 11, 2019

@nomeata I was going to suggest to have helpers for alloc+store_tag, etc. Will use the Tagged.obj helper!

the resulting code is
``` asm
          i32.const 2
          call $alloc_words
          tee_local $heap_object
          i32.const 11
          i32.store offset=1
          get_local $heap_object
          get_local $x
          i32.load offset=5
          i32.store offset=5
          get_local $heap_object
```
src/compile.ml Outdated Show resolved Hide resolved
@ggreif ggreif requested a review from nomeata March 11, 2019 16:48
Copy link
Collaborator

@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.

Thanks, good job!

@ggreif ggreif merged commit 9563d54 into master Mar 12, 2019
dfinity-bot added a commit that referenced this pull request Apr 8, 2021
mergify bot pushed a commit that referenced this pull request Apr 8, 2021
dfinity-bot added a commit that referenced this pull request Aug 11, 2023
## Changelog for ic-hs:
Branch: master
Commits: [dfinity/ic-hs@e727feca...a5f89cf5](dfinity/ic-hs@e727fec...a5f89cf)

* [`99efc33f`](dfinity/ic-hs@99efc33) sync node height before making an update call in compliance tests ([dfinity/ic-hs⁠#213](https://togithub.com/dfinity/ic-hs/issues/213))
* [`4bdf6c35`](dfinity/ic-hs@4bdf6c3) sync node height before getStateCert ([dfinity/ic-hs⁠#214](https://togithub.com/dfinity/ic-hs/issues/214))
* [`f718f74d`](dfinity/ic-hs@f718f74) retry on http status of 429 ([dfinity/ic-hs⁠#215](https://togithub.com/dfinity/ic-hs/issues/215))
* [`a5f89cf5`](dfinity/ic-hs@a5f89cf) add note that DFINITY stopped contributing ([dfinity/ic-hs⁠#216](https://togithub.com/dfinity/ic-hs/issues/216))
mergify bot pushed a commit that referenced this pull request Aug 11, 2023
## Changelog for ic-hs:
Branch: master
Commits: [dfinity/ic-hs@e727feca...a5f89cf5](dfinity/ic-hs@e727fec...a5f89cf)

* [`99efc33f`](dfinity/ic-hs@99efc33) sync node height before making an update call in compliance tests ([dfinity/ic-hs⁠#213](https://togithub.com/dfinity/ic-hs/issues/213))
* [`4bdf6c35`](dfinity/ic-hs@4bdf6c3) sync node height before getStateCert ([dfinity/ic-hs⁠#214](https://togithub.com/dfinity/ic-hs/issues/214))
* [`f718f74d`](dfinity/ic-hs@f718f74) retry on http status of 429 ([dfinity/ic-hs⁠#215](https://togithub.com/dfinity/ic-hs/issues/215))
* [`a5f89cf5`](dfinity/ic-hs@a5f89cf) add note that DFINITY stopped contributing ([dfinity/ic-hs⁠#216](https://togithub.com/dfinity/ic-hs/issues/216))
dfinity-bot added a commit that referenced this pull request Aug 27, 2024
## Changelog for ic-hs:
Branch: master
Commits: [dfinity/ic-hs@e727feca...a5f89cf5](dfinity/ic-hs@e727fec...a5f89cf)

* [`99efc33f`](dfinity/ic-hs@99efc33) sync node height before making an update call in compliance tests ([dfinity/ic-hs⁠#213](https://togithub.com/dfinity/ic-hs/issues/213))
* [`4bdf6c35`](dfinity/ic-hs@4bdf6c3) sync node height before getStateCert ([dfinity/ic-hs⁠#214](https://togithub.com/dfinity/ic-hs/issues/214))
* [`f718f74d`](dfinity/ic-hs@f718f74) retry on http status of 429 ([dfinity/ic-hs⁠#215](https://togithub.com/dfinity/ic-hs/issues/215))
* [`a5f89cf5`](dfinity/ic-hs@a5f89cf) add note that DFINITY stopped contributing ([dfinity/ic-hs⁠#216](https://togithub.com/dfinity/ic-hs/issues/216))
mergify bot pushed a commit that referenced this pull request Aug 27, 2024
## Changelog for ic-hs:
Branch: master
Commits: [dfinity/ic-hs@e727feca...a5f89cf5](dfinity/ic-hs@e727fec...a5f89cf)

* [`99efc33f`](dfinity/ic-hs@99efc33) sync node height before making an update call in compliance tests ([dfinity/ic-hs⁠#213](https://togithub.com/dfinity/ic-hs/issues/213))
* [`4bdf6c35`](dfinity/ic-hs@4bdf6c3) sync node height before getStateCert ([dfinity/ic-hs⁠#214](https://togithub.com/dfinity/ic-hs/issues/214))
* [`f718f74d`](dfinity/ic-hs@f718f74) retry on http status of 429 ([dfinity/ic-hs⁠#215](https://togithub.com/dfinity/ic-hs/issues/215))
* [`a5f89cf5`](dfinity/ic-hs@a5f89cf) add note that DFINITY stopped contributing ([dfinity/ic-hs⁠#216](https://togithub.com/dfinity/ic-hs/issues/216))
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