Improve type handling to work on 32-bit systems #20
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## master #20 +/- ##
=======================================
Coverage 99.35% 99.36%
=======================================
Files 1 1
Lines 468 469 +1
=======================================
+ Hits 465 466 +1
Misses 3 3 ☔ View full report in Codecov by Sentry. |
| if compression_mode == 0 | ||
| final_block = getbit(data) | ||
| compression_mode = getbits(data, 2) % UInt8 | ||
| if compression_mode === 0x0 |
There was a problem hiding this comment.
What is the point of this change? == 0 and === 0x0 generate the same code.
There was a problem hiding this comment.
The use of === implies that the compression_mode (etc.) is UInt8 (etc.).
In this case, it is obvious that compression_mode is converted to UInt8 above, but in general it is not obvious what type methods return.
Of course, there is another option of putting type annotations on variables, but I don't like the heavy use of annotations.
There was a problem hiding this comment.
How does it imply that? As far as I can tell the === comparison will just silently return false if the types differ and you need to rely on your test suite to detect that something goes wrong.
There was a problem hiding this comment.
Of course, there are no guarantees there, but what is wrong with that?
Are type annotations more preferable?
| if v < 256 | ||
| push!(out, UInt8(v)) | ||
| elseif v == 256 | ||
| if v < 0x100 |
There was a problem hiding this comment.
This makes no difference to the generated code and the RFC specifies it with decimal numbers.
There was a problem hiding this comment.
This PR changes are primarily intended to avoid undesirable promotions.
After optimization, the final native codes may be the same, but the IR codes should be different.
Of course, if you prefer decimal numbers, we can write UInt16(256) or LInt(256).
There was a problem hiding this comment.
Once it gets to llvm, at least the comparison in isolation ends up identical:
julia> f(x) = x < 256
f (generic function with 1 method)
julia> g(x) = x < 0x100
g (generic function with 1 method)
julia> code_llvm(f, (UInt16,))
; @ REPL[1]:1 within `f`
define i8 @julia_f_95(i16 zeroext %0) #0 {
top:
; ┌ @ int.jl:494 within `<` @ promotion.jl:450 @ int.jl:487
%1 = icmp ult i16 %0, 256
; └
%2 = zext i1 %1 to i8
ret i8 %2
}
julia> code_llvm(g, (UInt16,))
; @ REPL[2]:1 within `g`
define i8 @julia_g_106(i16 zeroext %0) #0 {
top:
; ┌ @ int.jl:487 within `<`
%1 = icmp ult i16 %0, 256
; └
%2 = zext i1 %1 to i8
ret i8 %2
}
There was a problem hiding this comment.
Fine. However should we even consider what optimizations the compiler will make?
There was a problem hiding this comment.
If it allows nicer looking code, I will consider that, yes.
There was a problem hiding this comment.
So, If you can foresee well enough what code the compiler will produce, why haven't we noticed the bug before?
I didn't notice it because I first read this code yesterday when I started debugging TiffImages.jl.
There was a problem hiding this comment.
Well, I've fixed the immediate problem in #21. Now the board is open for optimizations.
| const crc_table = zeros(UInt32, 256) | ||
|
|
||
| function make_crc_table() | ||
| for n = 1:256 | ||
| c = UInt32(n - 1) | ||
| for k = 1:8 | ||
| if (c & 0x00000001) != 0 | ||
| c = 0xedb88320 ⊻ (c >> 1) | ||
| c = 0xedb88320 ⊻ (c >> 0x1) |
There was a problem hiding this comment.
Also doesn't change the generated code.
| function init_crc() | ||
| if crc_table[1] == 0 | ||
| make_crc_table() | ||
| if crc_table[1] == 0 # FIXME: `crc_table[1]` is always zero. |
There was a problem hiding this comment.
Haha, I guess I took for granted that it would be something else.
This is the rework of PR #18, with the optimization-related parts removed.
This prevents undesirable promotion by integer literals. (Fixes #16)