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

LLVM checked arithmetic for 128-bit ints is broken #4905

Closed
StefanKarpinski opened this issue Nov 23, 2013 · 17 comments
Closed

LLVM checked arithmetic for 128-bit ints is broken #4905

StefanKarpinski opened this issue Nov 23, 2013 · 17 comments
Assignees
Labels
bug Indicates an unexpected problem or unintended behavior needs tests Unit tests are required for this change upstream The issue is with an upstream dependency, e.g. LLVM

Comments

@StefanKarpinski
Copy link
Member

This is largely an upstream bug tracking issue.

@StefanKarpinski
Copy link
Member Author

Oh, also 8-bit checked multiplies are also broken. Tracking that upstream bug here too.

@ViralBShah
Copy link
Member

It would be nice to have the upstream bug url here, if one has been filed.

@StefanKarpinski
Copy link
Member Author

I have not filed one. @loladiro has generally been our go-to guy for that interaction with the LLVM community.

@Keno
Copy link
Member

Keno commented Nov 23, 2013

I'll verify against trunk and file a bug report, no problem.

@StefanKarpinski
Copy link
Member Author

Thanks, Keno!

@JeffBezanson
Copy link
Member

Also broken for 64-bit ints on a 32-bit platform.
update: only 64-bit checked multiply is broken on 32-bit.

@Keno
Copy link
Member

Keno commented Jun 2, 2014

Fixed upstream by llvm-mirror/llvm@0bedfa4

@pao
Copy link
Member

pao commented Jun 17, 2015

Mentioning Int128 explicitly to make this easier to find.

@simonbyrne
Copy link
Contributor

What's the status of this? Are the other cases (Int128, Int64 on 32 bit) also fixed on new llvm?

@eschnett
Copy link
Contributor

Just checked on Ubuntu 14.04, 32-bit, with LLVM 3.7: Int64 and Int128 remain broken; the respective run-time functions (e.g. mulodi4, muloti4) are not found. With 64 bits, only Int128 is broken.

@Keno
Copy link
Member

Keno commented Dec 11, 2015

The codegen bug is fixed for all cases, but as mentioned there may still be missing run-time functions.

@simonbyrne
Copy link
Contributor

This has been monkey-patched by #14362. Should we close this or wait for the run-time functions?

@eschnett
Copy link
Contributor

#14362 didn't change anything for Julia users -- the work-around were already there before.

@tkelman tkelman added the needs tests Unit tests are required for this change label Jan 10, 2016
@vtjnash
Copy link
Member

vtjnash commented Jul 20, 2016

compiler-rt support will be fixed by #17344 allowing fixing of the underlying issue here

@StefanKarpinski
Copy link
Member Author

Fixed:

julia> a = rand(Int128)
168787286807142666394438386944935173819

julia> b = rand(Int128)
20332519185114748867952745427073576754

julia> a + b
-151162560928681048200983475059759460883

julia> Base.Checked.checked_add(a, b)
ERROR: OverflowError: 168787286807142666394438386944935173819 + 20332519185114748867952745427073576754 overflowed for type Int128
Stacktrace:
 [1] throw_overflowerr_binaryop(::Symbol, ::Int128, ::Int128) at ./checked.jl:154
 [2] checked_add(::Int128, ::Int128) at ./checked.jl:166
 [3] top-level scope at REPL[13]:1

I also checked UInt128 to be sure.

@oscardssmith
Copy link
Member

@StefanKarpinski since this is fixed, can

# LLVM has several code generation bugs for checked integer arithmetic (see e.g.
be cleaned up?

@StefanKarpinski
Copy link
Member Author

I dunno, I guess see if you can revert and it still works.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Indicates an unexpected problem or unintended behavior needs tests Unit tests are required for this change upstream The issue is with an upstream dependency, e.g. LLVM
Projects
None yet
Development

Successfully merging a pull request may close this issue.

10 participants