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

Fix codegen for sizeof for arrays with non-power-of-2 types #35900

Merged
merged 3 commits into from
May 17, 2020

Conversation

imciner2
Copy link
Contributor

@imciner2 imciner2 commented May 15, 2020

This fixes the code generation optimization for the sizeof() function to use the array element's alignment if the element is a primitive type instead of the size of the type. This is a fix for #35884. I also found a bug in the sizeof overload in the SubArrays type, so this is fixed here as well.

I have added in tests for the Array sizeof() call, but there are probably more that could be added (so different types are tested) - so any suggestions are appreciated.

Fixes #35884.

@rfourquet
Copy link
Member

In some circumstances sizeof is used to write or read the relevant number of bytes for a type, e.g. read at

function read(from::GenericIOBuffer, T::Union{Type{Int16},Type{UInt16},Type{Int32},Type{UInt32},Type{Int64},Type{UInt64},Type{Int128},Type{UInt128},Type{Float16},Type{Float32},Type{Float64}})
In that use-case, the old sizeof definition was "correct", so if sizeof is modified, it would be great to have an alternative function playing the role of the current sizeof.

I also don't know if the current definition of sizeof

Size, in bytes, of the canonical binary representation of the given DataType T

implies that the updated behavior here is the correct one, but it would be great to update the docstring to explain this subtlelty.

@imciner2 imciner2 changed the title Fix codegen for sizeof for non-power-of-2 types Fix codegen for sizeof for arrays with non-power-of-2 types May 15, 2020
@imciner2
Copy link
Contributor Author

imciner2 commented May 15, 2020

I should have been a bit clearer, this only changes the sizeof results for arrays containing these non-power-of-2 types. In this case, the data is actually aligned to be on a power of 2 anyway. This shouldn't affect the results for just primitive types (those will still return the correct bytes).

@rfourquet
Copy link
Member

Ah thanks for the clarification!

test/abstractarray.jl Outdated Show resolved Hide resolved
@JeffBezanson JeffBezanson added backport 1.5 bugfix This change fixes an existing bug compiler:codegen Generation of LLVM IR and native code labels May 15, 2020
@JeffBezanson JeffBezanson merged commit a383d61 into JuliaLang:master May 17, 2020
KristofferC pushed a commit that referenced this pull request May 19, 2020
mbauman added a commit that referenced this pull request Jul 17, 2020
Not all `AbstractArray`s define elsize.  Cf. #35900, #36714.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugfix This change fixes an existing bug compiler:codegen Generation of LLVM IR and native code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

sizeof reports wrong size for array of non-power-of-2 primitive types
4 participants