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

Implement lexer int128 support #11571

Merged

Conversation

BlobCodes
Copy link
Contributor

@BlobCodes BlobCodes commented Dec 11, 2021

Supersedes #11196
Related to #8373
Closes #7915
Related to #5545

This PR implements int128 support in the lexer.

@BlobCodes BlobCodes mentioned this pull request Dec 11, 2021
1 task
@HertzDevil
Copy link
Contributor

What is UInt64 doing here?

In src\lib_c\x86_64-windows-msvc\c\dbghelp.cr:6:33

 6 | SYMOPT_LOAD_LINES           = 0x00000010
                                   ^---------
Error: 0x00000010 doesn't fit in an UInt64

@BlobCodes
Copy link
Contributor Author

What is UInt64 doing here?

In src\lib_c\x86_64-windows-msvc\c\dbghelp.cr:6:33

 6 | SYMOPT_LOAD_LINES           = 0x00000010
                                   ^---------
Error: 0x00000010 doesn't fit in an UInt64

On Windows, the conversion from hex to dec fails, because the conversion is now handled using .to_u128?(base: 16).to_s (requires #11551).

The default error message when @token.value is nil and no suffix is given shows that it doesn't fit in the biggest integer (which should now be UInt128, I should change that).

@BlobCodes
Copy link
Contributor Author

I could also fall back to u64 on Windows and show that windows doesn't support u128 integers yet, but I think it's better to wait until it's supported everywhere.

@BlobCodes
Copy link
Contributor Author

BlobCodes commented Dec 12, 2021

Does this need to be changed to include U/Int128? (I don't really know much about codegen)

@compile_time_value : (Int16 | Int32 | Int64 | Int8 | UInt16 | UInt32 | UInt64 | UInt8 | Bool | Char | Nil)

EDIT: Oh, it's the integers coming from the mathinterpreter, which doesn't yet support int128.

@oprypin oprypin mentioned this pull request Dec 12, 2021
@oprypin
Copy link
Member

oprypin commented Dec 12, 2021

I think it's not fully obvious what type should be deduced for literals that don't have an explicit suffix.
I kicked off a discussion of that in the issue, please check it out:
#8373 (comment)

@BlobCodes
Copy link
Contributor Author

The newest commit allows constant math expressions to be interpreted at compile-time.
Basically, LLVM-IR before:

  call void @"~A:init"(), !dbg !24
  %57 = load i128, i128* @A, !dbg !24
  call void @"*puts<Int128>:Nil"(i128 %57), !dbg !91
  ret void, !dbg !91

After:

  call void @"*puts<Int128>:Nil"(i128 3), !dbg !97
  ret void, !dbg !97

Code:

A = 1_i128 + 2_i128
puts A

I hope the CI won't fail (except for windows)

@straight-shoota
Copy link
Member

Please move 128-bit math interpreter to a separate PR. It's unrelated to lexer support. And it needs specs.

@BlobCodes
Copy link
Contributor Author

The MathInterpreter changes (and a few more things) have been extracted to #11576

@oprypin
Copy link
Member

oprypin commented Dec 13, 2021

In terms of functionality, this is rock-solid. I just tested a huge range of numbers and it all matches expectations as per #8373 (comment) Alternative 4

See all 4004 passing specs' names

oprypin@3bb8a1a#diff-40baf545fe5167c85593fa7b286af7017cd0140411b0c12def7c995523d74b31R476

I codified all the number ranges in a shape that directly mirrors the tables from that comment.

test_cases("", [
{nil, "-2**127 - 1", "{} doesn't fit in an Int64"},
{"-2**127", "-2**63 - 1", "{} doesn't fit in an Int64, try using the suffix i128"},
{"-2**63", "-2**31 - 1", :i64},
{"-2**31", "2**31 - 1", :i32},
{"2**31", "2**63 - 1", :i64},
{"2**63", "2**64 - 1", :u64},
{"2**64", "2**127 - 1", "{} doesn't fit in an UInt64, try using the suffix i128"},
{"2**127", "2**128 - 1", "{} doesn't fit in an UInt64, try using the suffix u128"},
{"2**128", nil, "{} doesn't fit in an UInt64"},
])
[:i8, :i16, :i32, :i64, :i128].each do |suf|
size = suf.to_s[1..].to_i
test_cases(suf.to_s, [
{nil, "-2**#{size - 1} - 1", "{} doesn't fit in an Int#{size}"},
{"-2**#{size - 1}", "2**#{size - 1} - 1", suf},
{"2**#{size - 1}", nil, "{} doesn't fit in an Int#{size}"},
])
end
[:u8, :u16, :u32, :u64, :u128].each do |suf|
size = suf.to_s[1..].to_i
test_cases(suf.to_s, [
{nil, "-1", "Invalid negative value {} for UInt#{size}"},
{"0", "2**#{size} - 1", suf},
{"2**#{size}", nil, "{} doesn't fit in an UInt#{size}"},
])
end

Copy link
Member

@oprypin oprypin left a comment

Choose a reason for hiding this comment

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

After #11551 is merged (which it wasn't quite yet),
please merge master into this PR, to double-check CI on windows
and then let's merge this one :)
Thanks much! 🎉

@straight-shoota
Copy link
Member

#11551 has been merged.

@BlobCodes
Copy link
Contributor Author

All specs pass with #11551 🎉

@straight-shoota straight-shoota added this to the 1.3.0 milestone Dec 16, 2021
@straight-shoota straight-shoota merged commit bfddd26 into crystal-lang:master Dec 18, 2021
@BlobCodes BlobCodes deleted the feature/lexer-int128-support branch January 29, 2022 22:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug] Overflow in 128-bit negative integers
5 participants