-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
WIP: Generalize float intrinsics #3580
Conversation
Add support for FLoat16
Pretty cool. I no longer find it odd that types not used by Clang can be broken in LLVM. We really should work towards getting your patches accepted upstream. |
I posted this on an earlier version of this change:
Even though LLVM's Float16 support is broken, we should still merge the generalization stuff into master. To provide non-broken Float16 support, we can either
It might be time to start maintaining our own fork of LLVM, although that's a big undertaking. At the very least, we need to start pursuing getting fixes accepted into LLVM. It seems like Julia may be the premiere use case for "exotic" numeric types in LLVM at this point. |
@loladiro, how about separating this into two commits: one doing the generalization and a second adding the Float16 type? The first one can be merged into master as long as everything continues to work correctly, while the second should be a tiny patch that we can keep around until we figure out what to do. |
This is also the last set of intrinsics to be generalized to arbitrary size, aside from the mixed-type comparisons, which are unnecessary for smaller types like Float32 and thus only make sense for 64-bit (although we'll need larger versions if we ever add Float128 or other large floating-point types). |
@JeffBezanson There seems to be a lot of repeated code between generic_(un)box, try_to_determine_bitstype_nbits and staticeval_bitstype. Any subtle reason not to consolidate these, I'm missing? @StefanKarpinski Yes, I will split this patch. I didn't really get very far with Float16 support anyway, as it's completely unusable with current LLVM. |
Very good! How broken is Float16 in LLVM? Is it a small bug that can be fixed to make everything work, or is it a larger project? If it will take a while or isn't likely to be accepted, we can get started just by having the type and conversions to/from Float32 as Stefan said. |
Here's the upstream bug, for reference: http://llvm.org/bugs/show_bug.cgi?id=16491. The obvious problem is deceptively simple in that they simply made the assumption that 32bit is the smallest they'll ever encounter. Considering this is not at all tested in the LLVM codebase though, I wouldn't be surprised if more was broken. |
Alright. I switched the argument order and commented out the Float16 definitions, so this should be good to merge. I'll try to fix the Float16 issue in LLVM. Feel free to do whatever to Float16 in the mean time. |
I found the llvm-dev mailing dist discussion useful too: http://lists.cs.uiuc.edu/pipermail/llvmdev/2013-June/062968.html |
Is it possible to get this into 0.2, given the LLVM patches? |
The LLVM changes are only necessary to support Float16 correctly, so this can be merged without affecting behavior. |
I've tested and it seems all good and looks good to me. Travis say it's ok too, so @loladiro, go ahead an merge whenever you're ready. I think this should just go into master and we can add the Float16 stuff whenever/however. |
Merged! |
Stdlib: Pkg URL: https://github.com/JuliaLang/Pkg.jl.git Stdlib branch: master Julia branch: master Old commit: 047734e4c New commit: f570abd39 Julia version: 1.11.0-DEV Pkg version: 1.11.0 Bump invoked by: @KristofferC Powered by: [BumpStdlibs.jl](https://github.com/JuliaLang/BumpStdlibs.jl) Diff: JuliaLang/Pkg.jl@047734e...f570abd ``` $ git log --oneline 047734e4c..f570abd39 f570abd39 tweak how Pkg is loaded for precompiling when testing (#3606) d3bd38b90 sort compat entries in `pkg> compat` (#3605) 5e07cfed0 Ensure that `string(::VersionSpec)` is compatible with `semver_spec()` (#3580) 6bed7c41a Clarify handling of LOAD_PATH in docstring and REPL help (#3589) 5261b3be7 tweak test dep docs (#3574) 72b430d50 status: expand 2 symbol upgrade note to highlight *may* be upgradable (#3576) b32db473d precompile: show ext parent in output report (#3603) ``` Co-authored-by: Dilum Aluthge <[email protected]>
Unfortunately Float16 support is broken in LLVM (who could have guessed that a type not used by Clang is broken ...).
Anyway, once I fix that in LLVM, this closes #3467