Skip to content

Add workaround for 128-bit integer division/modulo on Windows#11551

Merged
straight-shoota merged 1 commit intocrystal-lang:masterfrom
HertzDevil:bug/windows-128bit-divmod
Dec 16, 2021
Merged

Add workaround for 128-bit integer division/modulo on Windows#11551
straight-shoota merged 1 commit intocrystal-lang:masterfrom
HertzDevil:bug/windows-128bit-divmod

Conversation

@HertzDevil
Copy link
Contributor

LLVM does not honor the Microsoft x64 ABI when it calls certain compiler-rt functions with 128-bit integer arguments from LLVM instructions, most notably the division and modulo intrinsics (sdiv, udiv, srem, urem). But now that the required compiler-rt functions are all defined in Crystal, we can make those calls in Crystal directly, as these functions have no ABI issues whether they are defined as defs or global funs.

This PR redefines the division and modulo primitive methods as non-primitives that call the respective compiler-rt functions on 64-bit Windows. The added overhead should be very small compared to those actual function bodies. The same calls will still break on an empty prelude, because crystal/compiler_rt is then not required, and because our ports so far require many definitions from src/int.cr.

@HertzDevil HertzDevil added kind:bug A bug in the code. Does not apply to documentation, specs, etc. platform:windows Windows support based on the MSVC toolchain / Win32 API topic:stdlib:numeric labels Dec 8, 2021
@straight-shoota
Copy link
Member

Is this a known LLVM bug? Can we fix LLVM to remove this workaround at some point?

@HertzDevil
Copy link
Contributor Author

HertzDevil commented Dec 8, 2021

We still have to keep this workaround around, guarded behind LibLLVM::IS_LT_*, as long as we support versions of LLVM that do not include a fix for this. And the approach above would be even more problematic because primitives.cr then depends on llvm/lib_llvm.cr.

@straight-shoota
Copy link
Member

straight-shoota commented Dec 8, 2021

But those are stdlib functions, we don't need libllvm for that. The comparsion would be with the LLVM version that the compiler was built with, which is Crystal::LLVM_VERSION. Or we could expose a compiler flag that indicates whether the workaround is necessary.

@oprypin
Copy link
Member

oprypin commented Dec 12, 2021

I don't understand the state of this PR. Is the last comment by @straight-shoota agreed upon and requires changes?

@HertzDevil
Copy link
Contributor Author

Apart from that there is also the issue that optimizations normally available to the corresponding LLVM instructions might not be available, instead they are optimized like normal function calls. Personally I'd like some more feedback from core members before deciding where to go with this.

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.

The current approach actually looks fine to me. If others think it's not perfect, do you see any issue with merging this first (and have int128 actually working) and figuring out the exact correct way later? As I understand, this approach doesn't bring any baggage that can't be changed at a later point. In fact, it cleans things up almost as much.

What's the concern with failing at runtime? Isn't it specific to empty prelude (i.e. pretty much just Crystal specs) and to int128 math and to Windows? Which is exceedingly rare.

Merging this would save a lot of work marking specs as pending in #11571 (not to mention possibly even blocking it outright).
And anyway it would be very good to have int128 support on all platforms even if Windows is just a preview.

@straight-shoota straight-shoota added this to the 1.3.0 milestone Dec 15, 2021
@straight-shoota straight-shoota merged commit afaae3f into crystal-lang:master Dec 16, 2021
@HertzDevil HertzDevil deleted the bug/windows-128bit-divmod branch December 16, 2021 11:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

kind:bug A bug in the code. Does not apply to documentation, specs, etc. platform:windows Windows support based on the MSVC toolchain / Win32 API topic:stdlib:numeric

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants