Skip to content

Add Compiler-RT for 128bit integer support#8313

Closed
jkthorne wants to merge 61 commits intocrystal-lang:masterfrom
jkthorne:i128_compiler_rt
Closed

Add Compiler-RT for 128bit integer support#8313
jkthorne wants to merge 61 commits intocrystal-lang:masterfrom
jkthorne:i128_compiler_rt

Conversation

@jkthorne
Copy link
Contributor

This is extracted from github.com//pull/7093.

This implements all the expected functions 128bit integer support.

@jkthorne
Copy link
Contributor Author

how can I make this a draft?

@bcardiff
Copy link
Member

You can’t. Draft can only be created and be marked as ready. No worries

@jkthorne
Copy link
Contributor Author

@bcardiff can you take a look at this when you get a chance?

@asterite
Copy link
Member

this PR is for Int128 but there is already a port of mulodi4 that is used to have Int64 overflow in 32 bits
compiler-rt is needed for 32 bit machines

But in #8283

Meanwhile, we are near to drop 32bits packages (and CI). LLVM 8 is not even available in their apt repo for 32 bits

So all the work we (as a community) put into compiler-rt go into:

  • Int128, for which there are no use cases and nobody asked it (I added it in the past because it seemed easy, but I was wrong)
  • Some operations in 32 bits machines, which will soon not be supported anymore

That's why I question why we need to continue putting effort in these things. It's not that I don't find this PR useful in its own, but given the context where it could be applied it seems to provide very little value.

@asterite
Copy link
Member

Regarding the many specs, I don't think we need one spec per operation. We can put all specs inside a single it block... if having multiple specs is a concern (I don't think it poses any problems).

@jkthorne
Copy link
Contributor Author

I was doing a project with Int128 integers a while ago and found that this was uncompleted.
I would like 128bit integer support in crystal and it seems like the majority if not all the work so far has gone into supporting 32 bit machines.

@jkthorne jkthorne mentioned this pull request Oct 24, 2019
@asterite
Copy link
Member

asterite commented Oct 24, 2019

@wontruefree What did you need Int128 in your project for?

@jkthorne
Copy link
Contributor Author

@asterite I was integrating with a cpp library that was using Int128. I was looking into slices and other technical solutions but in the end I discontinued the project. I picked this up because I thought it was interesting and I thought it would help us get to 1.0.

@asterite
Copy link
Member

Sorry for my interruption and discussion. If we can get this merged and support 32 bits and int128, I think this is the best result. Please go on!

test__muloti4(2097152_i128, Int128RT[4398046511103_i128].negate!, Int128RT[9223372036852678656_i128].negate!, 0)
test__muloti4(Int128RT[2097152_i128].negate!, Int128RT[4398046511103_i128].negate!, 9223372036852678656_i128, 0)

# test__muloti4(Int128RT[0x00000000000000B5_i64, 0x04F333F9DE5BE000_u64], Int128RT[0x0000000000000000_i64, 0x00B504F333F9DE5B_u64], Int128RT[0x7FFFFFFFFFFFF328_i64, 0xDF915DA296E8A000_u64], 0)
Copy link
Member

Choose a reason for hiding this comment

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

Why are there so many commented out tests?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

they are broken. The main problem is __udivmodti4 but it is hard to debug and I work on one failure at a time. I have gotten enough help from these PRs that I can close them and reopen them when I am closer if that helps.

Copy link
Member

@RX14 RX14 Nov 2, 2019

Choose a reason for hiding this comment

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

If this is WIP, then could you please indicate that in the title?

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.

6 participants