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

Int128 compiler-rt methods (Int128 literal support part 1) #11206

Merged

Conversation

BlobCodes
Copy link
Contributor

Includes the compiler-rt methods from #11196.

The modulo and dision methods do not work on windows due to the lack of SIMD support in crystal.

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

BlobCodes commented Sep 13, 2021

In rust's compiler-builtins, the method __muloti4 needed to be in extern "unadjusted" unstead of extern "C". Is it possible to do this in crystal?

AFAIK should be @[CallConvention("unadjusted")], but this is not a valid call convention in crystal.

The __muloti4 specs fail on windows, but this shouldn't need SIMD.

@asterite
Copy link
Member

It seems that's a calling convention in Rust, but I don't see it listed in LLVM: https://llvm.org/docs/LangRef.html#calling-conventions

I don't know what they call "unadjusted".

You can put a CallConvetion annotation to functions and methods:

https://crystal-lang.org/reference/syntax_and_semantics/annotations/built_in_annotations.html#callconvention

The values there should be updated.

Unfortunately I can't figure out what's this "unadjusted" in Rust... 🤔

@asterite
Copy link
Member

I found something: rust-lang/rust#38482 (comment)

Good luck!

(I still think we should drop support for Int128 and UInt128)

@BlobCodes
Copy link
Contributor Author

BlobCodes commented Sep 13, 2021

The muloti4 specs pass on my local vm (llvm 11.1)
Maybe this got fixed in a version between what the CI is using and 11.1 ?

I don't think it would be worth fixing this if this is the case

@straight-shoota
Copy link
Member

Specs fail on win32 with both LLVM 10.0 and 11.1 for me. So it appears to be something different. But don't worry about that.
We've already established that Windows support is a sizable task. That should be worked on on its own, after this PR has landed.

@BlobCodes
Copy link
Contributor Author

Specs fail on win32 with both LLVM 10.0 and 11.1 for me. So it appears to be something different. But don't worry about that.
We've already established that Windows support is a sizable task. That should be worked on on its own, after this PR has landed.

Just checked again, looks like I forgot to replace the win32_std_spec.cr 😄
Yeah, it does fail on 11.1.

@BlobCodes
Copy link
Contributor Author

I think this should be ready to review now

@asterite
Copy link
Member

Add now-possible int operations to specs

I think this one should be reverted. We shouldn't use new compiler features in std specs. Otherwise you can't run std specs without compiling a new compiler.

@asterite
Copy link
Member

(though maybe that changed, no idea... if CI passes it's fine, maybe someone changed that rule...)

@BlobCodes
Copy link
Contributor Author

(though maybe that changed, no idea... if CI passes it's fine, maybe someone changed that rule...)

Everything necessary to pass the specs is in the prelude, the compiler source hasn't changed.
Should be fine AFAIK

spec/std/crystal/compiler_rt/divmod128_spec.cr Outdated Show resolved Hide resolved
spec/std/int_spec.cr Outdated Show resolved Hide resolved
Co-authored-by: Johannes Müller <straightshoota@gmail.com>
Copy link
Member

@beta-ziliani beta-ziliani left a comment

Choose a reason for hiding this comment

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

Thanks!

@straight-shoota straight-shoota added this to the 1.2.0 milestone Sep 23, 2021
@straight-shoota straight-shoota merged commit ae915ae into crystal-lang:master Sep 24, 2021
@BlobCodes BlobCodes deleted the int128-compiler-rt-methods branch January 29, 2022 22:39
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.

5 participants