Operations between Float16 and Integer now return Float16 #17297

vchuravy · 2016-07-06T14:14:46Z

Previously we would promote to Float32 leading to subtle type
instabilities when Float16 was used for computations instead of
a pure data-storage format.

Fixes #17261

vchuravy · 2016-07-06T19:58:22Z

By only defining promote_type(Float16, Integer) = Float16 this
would incur an overhead by first converting the Integer argument
to Float16 and then Float32. Since currently all mathematical
operations fall back to Float32 this converts the types as early
as possible.

This might be problematic for platforms that natively implement
Float16 support and maybe the AVX SIMD instructions that can
support Float16.

vchuravy · 2016-07-10T03:10:58Z

Removed the fancy optimizations so this will be consistent, but slower.

vchuravy · 2016-07-11T20:14:06Z

This is the minimal change necessary to make this work or am I missing something?

I would be in favour of doing this now and making it fast later.

StefanKarpinski · 2016-07-11T20:52:45Z

👍

tkelman · 2016-07-12T08:46:05Z

NEWS.md

@@ -107,6 +107,8 @@ Breaking changes
  * Juxtaposition of numeric literals ending in `.` (e.g. `1.x`) is no longer
    allowed ([#15731]).

+  * Operations between `Float16` and `Integers` now return `Float16` instead of `Float32`. ([#17261])


tkelman · 2016-07-14T19:22:21Z

@vchuravy I rebased this because of the Float16 inference issues (#17394), and turns out this introduced more failing cases. Hopefully one of the compiler gurus can fix that issue? #17313 passed its tests when they ran on CI after the most recent PR push a few days ago, but not after it was merged due to some intervening change on master that introduced an inference regression.

The failures after my commit here are an unrelated appveyor timeout and osx libgit2 segfault (problematic libgit2 change was reverted). I guess we could merge this unless someone knows how to fix the inference issue.

tkelman · 2016-07-14T19:24:20Z

test/numbers.jl

+                        (S in (Int, Rational{Int})) ||
+                        (S === Complex{Int} && op in (+, -, *, ^)) ||
+                        (Complex{UInt} in (R, S) && op === ^) ||
+                        (S <: Integer && op === ^)


This is really messy and took more time to get passing than I should have spent on it. We should maybe comment out Float16 and Complex{Float16} from the loop if this is hard to fix, though we won't know when it gets fixed if we do that.

See #17389, that should fix all these.

vchuravy · 2016-07-15T21:11:52Z

@tkelman Thanks for picking this up. I sadly don't know how to fix the test failures... I would like to see this in 0.5 so that we can work on #17440

tkelman · 2016-07-18T13:03:56Z

This was a bit unconventional to bisect, had to apply the patch from https://github.com/JuliaLang/julia/commit/6b40f865f0f379babea6daa0729fc3afbfc5c301.patch at different points in the history to see where it would cause @code_warntype Float16(1)+true to start giving ANY output. Turns out it was cd35df5. Is there a comprehension somewhere in the promotion call chain for Float16 conversion?

vchuravy · 2016-07-18T14:40:36Z

Not as far as I can tell, but this makes the call-chain a lot longer and we
do a lot more conversions to Float16. I just stepped through the entire
chain with Gallium for

x = Float16(1.0)
y = Rational{Int64}(1, 2)
@enter x + y

and we convert a lot more and a lot earlier to and fromFloat16, but there
is no comprehension in sight.

vchuravy · 2016-07-18T14:53:26Z

base/float16.jl

@@ -160,7 +161,7 @@ end

 ldexp(a::Float16, b::Integer) = Float16(ldexp(Float32(a), b))

-^(x::Float16, y::Integer) = Float16(Float32(x)^y)
+^(x::Float16, y::Float16) = Float16(Float32(x)^Float32(y))


This change is a mistake.

^(x::Float16, y::Integer) = Float16(Float32(x)^y) was correct.

JeffBezanson · 2016-07-22T20:57:07Z

Bump. Rebase and merge?

tkelman · 2016-07-22T21:02:25Z

I think this would violate feature freeze. No more breaking changes until we branch.

StefanKarpinski · 2016-07-22T22:43:19Z

It's really a shame that this slipped through the cracks, but yes, at this point this violates feature freeze.

ViralBShah · 2016-07-23T18:40:25Z

Would doing it in 0.5.x be possible - I guess this is strictly not a breaking change. If this is considered breaking and won't happen in 0.5.x, then we should try and get it in now rather than go a whole release with the old behaviour.

tkelman · 2016-07-23T18:47:46Z

We wouldn't backport something that changes return types either. We need to stop putting anything other than bug fixes (or docs, or simple test additions) in this release, that's what feature freeze means.

StefanKarpinski · 2016-07-23T18:50:31Z

If this isn't in 0.5.0, it can't be in 0.5.x – it's very much a behavior change that is not a bug fix. Maybe we should just leave this and make a concerted effort in the next release to make sure that Float16s uniformly work just like Float32 and Float64. I suspect there are more behavioral differences than this.

Previously we would promote to Float32 leading to subtle type instabilities when Float16 was used for computations instead of a pure data-storage format. By defining `promote_type(Float16, Integer) = Float16` this incurs an overhead by first converting the `Integer` argument to `Float16` and then `Float32` for mathematical operations, that are performed in Float32.

vchuravy · 2016-08-05T17:52:51Z

Yeah! The tests pass.

tkelman · 2016-08-05T18:47:51Z

NEWS.md

@@ -160,6 +160,8 @@ This section lists changes that do not have deprecation warnings.
  * `map` on a dictionary now expects a function that expects and returns a `Pair`.
    The result is now another dictionary instead of an array ([#16622]).

+  * Operations between `Float16` and `Integers` now return `Float16` instead of `Float32`. ([#17261])


should be in 0.6 news section now

[ci skip]

kshyatt added the types and dispatch Types, subtyping and method dispatch label Jul 6, 2016

vchuravy mentioned this pull request Jul 8, 2016

[RFC/WIP] Leverage compiler-rt for builtins support of llvm intrinsics #17344

Closed

3 tasks

tkelman reviewed Jul 12, 2016
View reviewed changes

tkelman reviewed Jul 14, 2016
View reviewed changes

vchuravy reviewed Jul 18, 2016
View reviewed changes

timholy mentioned this pull request Jul 21, 2016

Make conversions work for Float16 (and speed tests) JuliaMath/FixedPointNumbers.jl#43

Merged

vchuravy added 2 commits August 5, 2016 23:59

enable two additional Float16 tests

fdd4f6f

tkelman reviewed Aug 5, 2016
View reviewed changes

Start v0.6 release notes

d187cdb

[ci skip]

tkelman merged commit 2156195 into JuliaLang:master Aug 6, 2016

vchuravy mentioned this pull request Sep 26, 2016

Move Float16 functions away from float16.jl to places where other flo… #18287

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Operations between Float16 and Integer now return Float16 #17297

Operations between Float16 and Integer now return Float16 #17297

vchuravy commented Jul 6, 2016

vchuravy commented Jul 6, 2016

vchuravy commented Jul 10, 2016

vchuravy commented Jul 11, 2016

StefanKarpinski commented Jul 11, 2016

tkelman Jul 12, 2016

tkelman commented Jul 14, 2016

tkelman Jul 14, 2016

pabloferz Jul 20, 2016

vchuravy commented Jul 15, 2016

tkelman commented Jul 18, 2016

vchuravy commented Jul 18, 2016 •

edited

Loading

vchuravy Jul 18, 2016

JeffBezanson commented Jul 22, 2016

tkelman commented Jul 22, 2016

StefanKarpinski commented Jul 22, 2016

ViralBShah commented Jul 23, 2016

tkelman commented Jul 23, 2016 •

edited

Loading

StefanKarpinski commented Jul 23, 2016

vchuravy commented Aug 5, 2016

tkelman Aug 5, 2016

Operations between Float16 and Integer now return Float16 #17297

Operations between Float16 and Integer now return Float16 #17297

Conversation

vchuravy commented Jul 6, 2016

vchuravy commented Jul 6, 2016

vchuravy commented Jul 10, 2016

vchuravy commented Jul 11, 2016

StefanKarpinski commented Jul 11, 2016

tkelman Jul 12, 2016

Choose a reason for hiding this comment

tkelman commented Jul 14, 2016

tkelman Jul 14, 2016

Choose a reason for hiding this comment

pabloferz Jul 20, 2016

Choose a reason for hiding this comment

vchuravy commented Jul 15, 2016

tkelman commented Jul 18, 2016

vchuravy commented Jul 18, 2016 • edited Loading

vchuravy Jul 18, 2016

Choose a reason for hiding this comment

JeffBezanson commented Jul 22, 2016

tkelman commented Jul 22, 2016

StefanKarpinski commented Jul 22, 2016

ViralBShah commented Jul 23, 2016

tkelman commented Jul 23, 2016 • edited Loading

StefanKarpinski commented Jul 23, 2016

vchuravy commented Aug 5, 2016

tkelman Aug 5, 2016

Choose a reason for hiding this comment

vchuravy commented Jul 18, 2016 •

edited

Loading

tkelman commented Jul 23, 2016 •

edited

Loading