Clean up implementation of checked integer arithmetic #14362

eschnett · 2015-12-11T03:11:18Z

Move checked_* implementation in Base into a module Checked in a new file checked.jl
Provide missing checked_* functions for BigInt (BigInt always checks)
Provide missing type promotions for checked_* functions
Move all checked_* tests into their on file test/checked.jl
Clean up handling LLVM codegen bugs
Introduce new function jl_get_LLVM_VERSION which works already early during bootstrap
Rename intrinsics to have an "_int" suffix, like all other integer intrinsics
In coreimg.jl, emulate checked arithmetic by regular (unchecked) arithmetic

tkelman · 2015-12-11T03:32:28Z

test/checked.jl

@@ -0,0 +1,224 @@
+# This file is a part of Julia. License is MIT: http://julialang.org/license
+
+# Checked integer arithmetic


needs to be added to choosetests.jl or it won't run

eschnett · 2015-12-11T18:44:58Z

Tests are now running, moved LLVM-specific function to intrinsics.cpp.

tkelman · 2015-12-11T21:37:14Z

src/intrinsics.cpp

@@ -35,6 +35,18 @@ static void jl_init_intrinsic_functions_codegen(Module *m)
 #undef ALIAS
 }

+extern "C" {
+  JL_DLLEXPORT uint32_t jl_get_LLVM_VERSION(void);


don't really need the braces for a single function

4 space indent in the function body

eschnett · 2015-12-11T23:53:29Z

Corrected indentation.

simonbyrne · 2015-12-14T09:57:07Z

base/checked.jl

+# typealias BrokenUnsignedIntMul UnsignedInt
+
+# A tight `widen`
+nextwide(::Type{Int8}) = Int16


What's the advantage of nextwide over widen?

widen jumps right away to Int64, which, if you are going to be allocating an array, would be very wasteful.

But why is it useful in this case?

It's useful for two reasons:

32-bit arithmetic may be cheaper than 64-bit arithmetic, in particular for multiplication and division.

If (my dream!) the compiler auto-vectorizes the code, then smaller types allow packing more operations into a vector. 64-bit integer arithmetic is very difficult to vectorize on x86, 32-bit integer arithmetic is much easier.

I see your point, but if there are genuine gains to be had (some benchmarks would be useful), then we should modify widen and widemul more generally. Just having it here is somewhat odd.

See http://www.agner.org/optimize/instruction_tables.pdf for raw benchmarks for various instructions on Intel architecture. I'm looking e.g. at page 188 for the Haswell microarchitecture. There we find for idiv (signed integer division):

32 bit: 22-29 cycles latency, 8-11 cycles inverse throughput (i.e. cycles/instruction)

64 bit: 39-103 cycles, 24-81 cycles inverse throughput
I've ran a "real" benchmark as well, and can roughly confirm these numbers. On my system, I find

32 bit: about 8 cycles/instruction

64 bit: about 26 cycles/instruction

Should we change the definition of widen, or should we introduce nextwide into Base? Should widen be system-dependent, or only distinguish between 32-bit and 64-bit word sizes?

According to Agner's table, there is no real benefit to go below 32 bits on a modern 64-bit Intel architecture.

I think this is a good argument to change the behaviour of widen (I don't think we want 2 functions which do basically the same thing), but I think that should be a separate pull request.

(In this case, the widening is only used for multiplication, and the performance stats are the same for 32/64bit).

eschnett · 2015-12-14T22:47:41Z

I'm now using widen, and also r %T for performance.

simonbyrne · 2015-12-14T22:51:47Z

base/checked.jl

+    box(T, checked_umul_int(unbox(T,x), unbox(T,y)))
+end
+function checked_mul{T<:BrokenSignedIntMul}(x::T, y::T)
+    r = widen(x) * widen(y)


I don't mean to be pedantic, but you can use widemul for this.

jakebolewski · 2015-12-16T23:07:39Z

LGTM. @tkelman the "checked" tests are included in choosetests.jl. Is this waiting on #14414?

eschnett · 2015-12-17T02:28:44Z

@jakebolewski No, this is independent.

simonbyrne · 2015-12-17T14:52:12Z

LGTM as well.

- Move checked_* implementation in Base into a module Checked in a new file checked.jl - Provide missing checked_* functions for BigInt (BigInt always checks) - Provide missing type promotions for checked_* functions - Move all checked_* tests into their on file test/checked.jl - Clean up handling LLVM codegen bugs - Introduce new function jl_get_LLVM_VERSION which works already early during bootstrap - Rename intrinsics to have an "_int" suffix, like all other integer intrinsics - In coreimg.jl, emulate checked arithmetic by regular (unchecked) arithmetic

eschnett · 2015-12-17T19:17:28Z

Rebased.

Clean up implementation of checked integer arithmetic

JeffBezanson · 2015-12-17T22:40:40Z

base/int.jl

-    div(x::Int128, y::Int128) = Int128(div(BigInt(x),BigInt(y)))
-    div(x::UInt128, y::UInt128) = UInt128(div(BigInt(x),BigInt(y)))
+    function div(x::Int128, y::Int128)
+        try


Using try/catch here is going to be really slow. Would be better to use a test and branch for the one invalid case.

See #14436.

tkelman reviewed Dec 11, 2015
View reviewed changes

eschnett force-pushed the eschnett/cleanup-checked-2 branch 4 times, most recently from eb0f021 to 56bf256 Compare December 11, 2015 05:21

tkelman reviewed Dec 11, 2015
View reviewed changes

simonbyrne reviewed Dec 14, 2015
View reviewed changes

eschnett mentioned this pull request Dec 14, 2015

widen should not widen to more than 32 bits unless necessary #14407

Closed

simonbyrne reviewed Dec 14, 2015
View reviewed changes

eschnett force-pushed the eschnett/cleanup-checked-2 branch from feb085d to 8d7387d Compare December 17, 2015 18:12

simonbyrne added a commit that referenced this pull request Dec 17, 2015

Merge pull request #14362 from eschnett/eschnett/cleanup-checked-2

ec609cc

Clean up implementation of checked integer arithmetic

simonbyrne merged commit ec609cc into JuliaLang:master Dec 17, 2015

eschnett deleted the eschnett/cleanup-checked-2 branch December 17, 2015 20:13

This was referenced Dec 17, 2015

Reliability of rational arithmetic #11736

Closed

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

Closed

JeffBezanson reviewed Dec 17, 2015
View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Clean up implementation of checked integer arithmetic #14362

Clean up implementation of checked integer arithmetic #14362

eschnett commented Dec 11, 2015

tkelman Dec 11, 2015

eschnett commented Dec 11, 2015

tkelman Dec 11, 2015

eschnett commented Dec 11, 2015

simonbyrne Dec 14, 2015

ScottPJones Dec 14, 2015

simonbyrne Dec 14, 2015

eschnett Dec 14, 2015

simonbyrne Dec 14, 2015

eschnett Dec 14, 2015

simonbyrne Dec 14, 2015

eschnett commented Dec 14, 2015

simonbyrne Dec 14, 2015

jakebolewski commented Dec 16, 2015

eschnett commented Dec 17, 2015

simonbyrne commented Dec 17, 2015

eschnett commented Dec 17, 2015

JeffBezanson Dec 17, 2015

eschnett Dec 18, 2015

		@@ -0,0 +1,224 @@
		# This file is a part of Julia. License is MIT: http://julialang.org/license

		# Checked integer arithmetic

Clean up implementation of checked integer arithmetic #14362

Clean up implementation of checked integer arithmetic #14362

Conversation

eschnett commented Dec 11, 2015

Choose a reason for hiding this comment

eschnett commented Dec 11, 2015

Choose a reason for hiding this comment

eschnett commented Dec 11, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eschnett commented Dec 14, 2015

Choose a reason for hiding this comment

jakebolewski commented Dec 16, 2015

eschnett commented Dec 17, 2015

simonbyrne commented Dec 17, 2015

eschnett commented Dec 17, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment