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

Support U/Int128 parsing and U/Int128 literals #11111

Closed
wants to merge 22 commits into from

Conversation

BlobCodes
Copy link
Contributor

@BlobCodes BlobCodes commented Aug 20, 2021

This PR adds:

  • String#to_(u/i)128(?) methods
  • (U)Int128 literal parsing
  • (U)Int128 modulo/divide/multiply-overflow methods in compiler-rt (to allow this to even work)

With this you can do crazy things like:

$ bin/crystal eval 'puts 0xFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF_u128 - 1'
Using compiled compiler at .build/crystal
340282366920938463463374607431768211454

To not introduce breaking changes, the method deduce_integer_kind now uses the following priority:
Int32 > Int64 > UInt64 > Int128 > UInt128. I don't know if this should be changed.

Requires/Includes #11093
Related to #8373
Supersedes #10975
Closes #9516
Closes #10962
Closes #7915
Related to #5545

@jkthorne
Copy link
Contributor

I ❤️ this. I would love to see U/Int128 support in Crystal.

@BlobCodes
Copy link
Contributor Author

BlobCodes commented Aug 20, 2021

A lot of checks seem to fail because they try to run the specs with the current release of crystal.
Of course all those UInt128 tests fail using the old compiler that doesn't support it.

When I run make crystal and then make std_spec, it always works.

@BlobCodes
Copy link
Contributor Author

I just noticed there is a little breaking change:

$ crystal eval 'puts 0b_u32'
0

$ bin/crystal eval 'puts 0b_u32'
syntax error in eval:1
Error: numbers cannot end with a prefix

I could easily change that behaviour however.

@jkthorne
Copy link
Contributor

I think you might need specs. I took a few swings at specs in some older PRs but I think you would probably want to take a slightly different approach. Probably convert the test cases into the base10 number and do the assertions.

#7093
#8313

Also when I last worked on this I had to update some code in the symantics and syntax. Have you tried using the Int128 in a program. Does it work as expected?

@BlobCodes
Copy link
Contributor Author

BlobCodes commented Aug 20, 2021

I think you might need specs. I took a few swings at specs in some older PRs but I think you would probably want to take a slightly different approach. Probably convert the test cases into the base10 number and do the assertions.

#7093
#8313

Also when I last worked on this I had to update some code in the symantics and syntax. Have you tried using the Int128 in a program. Does it work as expected?

I have added specs (compiler specs and std specs), but I have only tested some basic math operations, nothing complicated. I'll probably need to do that.

EDIT: Oh I just saw that there are specs for compiler-rt aswell.. I will probably need specs there too.

@BlobCodes
Copy link
Contributor Author

Now there are specs for the new compiler-rt methods!

@BlobCodes
Copy link
Contributor Author

Everything should work now, specs are also there.
Reviews would now be very appreciated!

@BlobCodes
Copy link
Contributor Author

BlobCodes commented Aug 22, 2021

The only issue right now is the windows CI.
It returns the error "170141183460469231731687303715884105727_i128 doesn't fit in an Int128", which is wrong..

Could someone who knows anything about crystal windows support look into this?

@BlobCodes
Copy link
Contributor Author

Alright.. after some testing it seems like windows is a major obstacle.

Here's a demonstration to show how bad the situation is (both crystal-master + my branch):

C:\crystal>crystal eval "puts 4_i128"
0

@BlobCodes
Copy link
Contributor Author

Windows really doesn't like my division/modulo implementation.

C:\crystal>crystal eval "puts (10.to_i128 // 2.to_i128).unsafe_as(Tuple(Int64, Int64))"
{0, 0}

C:\crystal>crystal eval "puts (10.to_i128 % 2.to_i128).unsafe_as(Tuple(Int64, Int64))"
{0, 0}

C:\crystal>crystal eval "puts (10.to_i128 * 2.to_i128).unsafe_as(Tuple(Int64, Int64))"
{20, 0}

C:\crystal>crystal eval "puts (10.to_i128 << 2.to_i128).unsafe_as(Tuple(Int64, Int64))"
{40, 0}

But at least I know where to search for errors now..

@BlobCodes
Copy link
Contributor Author

Okay, appearantly windows doesn't call my compiler-rt methods.
Using them explicitly works.

C:\crystal>crystal eval "puts __udivti3(UInt128::MAX.to_u128,32.to_u128).unsafe_as(Tuple(UInt64, UInt64))"
{18446744073709551615, 576460752303423487}

Co-authored-by: Sijawusz Pur Rahnama <[email protected]>
@BlobCodes
Copy link
Contributor Author

@BlobCodes
Copy link
Contributor Author

It seems like msvc passes the u/int128's using a pointer to compiler-rt.
Now my compiler-rt methods finally have the numbers they need and can correctly calculate the result.

The only problem now is that I have no clue how to return the calculated numbers.

@straight-shoota
Copy link
Member

Thanks for this PR @BlobCodes, it's really awesome!

In order to get this merged, we'll have to split it into two stages. The first stage adding compiler support and all the stdlib requirements for that. Only after that's been merged and released we can then merge the changes that actually make use of those compiler features.

I wouldn't worry too much about windows support for now. It seems more complicated to work our. We can defer that after merging the feature for other platforms. It's not a requirement. You can just use flags to disable failing pieces on win32.

There should be more unit test for overflowing multiplication. There is already mulodi4_spec.cr, we should have respective ones for mulosi4 and muloti4. I actually started working on the latter, you can grab the code from straight-shoota@2f714ec .

@BlobCodes
Copy link
Contributor Author

Thanks for this PR @BlobCodes, it's really awesome!

In order to get this merged, we'll have to split it into two stages. The first stage adding compiler support and all the stdlib requirements for that. Only after that's been merged and released we can then merge the changes that actually make use of those compiler features.

I wouldn't worry too much about windows support for now. It seems more complicated to work our. We can defer that after merging the feature for other platforms. It's not a requirement. You can just use flags to disable failing pieces on win32.

There should be more unit test for overflowing multiplication. There is already mulodi4_spec.cr, we should have respective ones for mulosi4 and muloti4. I actually started working on the latter, you can grab the code from straight-shoota@2f714ec .

Thanks, that's helpful!
3 _muloti4 specs actually failed because it uses the 128-bit division algorithm, in which I had mistakenly swapped two variables..
Now everything passes locally, will create the PR soon

@BlobCodes BlobCodes mentioned this pull request Sep 9, 2021
1 task
@BlobCodes
Copy link
Contributor Author

Superseded by #11196

@BlobCodes BlobCodes closed this Sep 11, 2021
@asterite
Copy link
Member

@straight-shoota I honestly think the most fair thing to do is for us, core team members, to take care of i128 support, at least for multiplying them. And that can be in a separate PR. Mixing that with parsing Int128 doesn't feel right. Why don't we break them appart?

@straight-shoota
Copy link
Member

straight-shoota commented Sep 12, 2021

Not sure if that's particularly fair, but yes of course, we can take care of that. But I have the impression that @BlobCodes has fun working on this, so why take it away?
Obviously, if nobody was working on that (or stops working), we should take it. Especially for the more complex tasks.

But as long as others want to work on something, I think our role should be primarly to support them to get their patches ready and merged.
However, of course nobody should feel responsible for finishing what they started, especially when the tasks grow while being worked on. Don't hesitate to step away if you don't want keep going.

In summary, my understanding of the core team's responsibility is to help other make contributions and take on those tasks that nobody else does =)

@straight-shoota
Copy link
Member

Regarding the specific note about i128 arithmetics, I think that's really already completed in #11196. Except for windows support which I suggested to postpone (and I'm happy to work on afterwards, if nobody else is interested).

@BlobCodes
Copy link
Contributor Author

Regarding the specific note about i128 arithmetics, I think that's really already completed in #11196. Except for windows support which I suggested to postpone (and I'm happy to work on afterwards, if nobody else is interested).

Supporting the int128 arithmetics is not the problem.
The problem is that windows requires the divti/modti methods to return the integer in the xmm0 register.
To get it there, crystal needs SIMD support.

Basically, we'd need to do this:

{% if flag?(:win32) %}
  # Windows
  @[SIMD]
  record U64x2, a : UInt64, b : UInt64
  fun __divti3(a : Int128*, b : Int128*) : U64x2
{% else %}
# Unix
  fun __divti3(a : Int128, b : Int128) : Int128
{% end %}

It would be really nice if the core members could add this, because I have no clue about crystal codegen and llvm (I think there was already a implementation in a crystal issue)

@asterite
Copy link
Member

@BlobCodes It seems SIMD in llvm is represented by the vector type: https://llvm.org/docs/LangRef.html#vector-type . It's almost like a StaticArray, except that it uses a different internal representation. I guess creating a type for it should be pretty easy. The hardest part will be to come up with a name for it 😁

But I was talking about how to dissect this PR. The interpreter doesn't work right now in linux because multiplication between Int128 doesn't work. I think a PR that just tackles that makes sense. It has nothing to do with parsing Int128. That's all I was saying. And for that, there's no need for you, @BlobCodes, to do it. The code is already in some of these PRs. One core team member could extract those changes and add specs on their own.

@BlobCodes BlobCodes deleted the int128-parsing branch January 29, 2022 22:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants