Skip to content

Commit

Permalink
Fix correctness issues in sizeof tfunc (#36727)
Browse files Browse the repository at this point in the history
The `Core.sizeof` function can take either a value or a type,
which can be a bit confusing in the tfunc, because the tfunc
operates on the types of the values passed to the builtin.
In particular, we were incorrectly returning Const(16) (on 64bit)
for sizeof_tfunc(UnionAll), because it was treating it the asme
as `sizeof_tfunc(Const(UnionAll))`. Try to clean that up as well
as fixing a similar confusion in the nothrow version of this
function. Lastly, we had a similar fast path in codegen, which
would try to inline the actual size for a constant datatype argument.
For codegen, just rm that whole code path, since it should have no
more information than inference and figuring it out in inference
exposes strictly more optimization opportunities.

Fixes #36710
  • Loading branch information
Keno committed Jul 20, 2020
1 parent 2cca0a4 commit 004cb25
Show file tree
Hide file tree
Showing 5 changed files with 79 additions and 36 deletions.
8 changes: 3 additions & 5 deletions base/array.jl
Original file line number Diff line number Diff line change
Expand Up @@ -211,11 +211,9 @@ julia> Base.bitsunionsize(Union{Float64, UInt8, Int128})
```
"""
function bitsunionsize(u::Union)
sz = Ref{Csize_t}(0)
algn = Ref{Csize_t}(0)
isunboxed = ccall(:jl_islayout_inline, Cint, (Any, Ptr{Csize_t}, Ptr{Csize_t}), u, sz, algn)
@assert isunboxed != Cint(0)
return sz[]
isinline, sz, _ = uniontype_layout(u)
@assert isinline
return sz
end

length(a::Array) = arraylen(a)
Expand Down
49 changes: 41 additions & 8 deletions base/compiler/tfuncs.jl
Original file line number Diff line number Diff line change
Expand Up @@ -302,16 +302,34 @@ function sizeof_nothrow(@nospecialize(x))
end
elseif isa(x, Conditional)
return true
else
x = widenconst(x)
end
if isa(x, Union)
return sizeof_nothrow(x.a) && sizeof_nothrow(x.b)
end
isconstType(x) && (x = x.parameters[1]) # since sizeof(typeof(x)) == sizeof(x)
x === DataType && return false
return isconcretetype(x) || isprimitivetype(x)
t, exact = instanceof_tfunc(x)
if !exact
# Could always be bottom at runtime, which throws
return false
end
if t !== Bottom
t === DataType && return true
x = t
x = unwrap_unionall(x)
if isa(x, Union)
isinline, sz, _ = uniontype_layout(x)
return isinline
end
isa(x, DataType) || return false
x.layout == C_NULL && return false
(datatype_nfields(x) == 0 && !datatype_pointerfree(x)) && return false
return true
else
x = widenconst(x)
x === DataType && return false
return isconcretetype(x) || isprimitivetype(x)
end
end

function _const_sizeof(@nospecialize(x))
# Constant Vector does not have constant size
isa(x, Vector) && return Int
Expand All @@ -330,12 +348,27 @@ function sizeof_tfunc(@nospecialize(x),)
isa(x, Const) && return _const_sizeof(x.val)
isa(x, Conditional) && return _const_sizeof(Bool)
isconstType(x) && return _const_sizeof(x.parameters[1])
x = widenconst(x)
if isa(x, Union)
return tmerge(sizeof_tfunc(x.a), sizeof_tfunc(x.b))
end
x !== DataType && isconcretetype(x) && return _const_sizeof(x)
isprimitivetype(x) && return _const_sizeof(x)
# Core.sizeof operates on either a type or a value. First check which
# case we're in.
t, exact = instanceof_tfunc(x)
if t !== Bottom
# The value corresponding to `x` at runtime could be a type.
# Normalize the query to ask about that type.
x = unwrap_unionall(t)
if isa(x, Union)
isinline, sz, _ = uniontype_layout(x)
return isinline ? Const(Int(sz)) : (exact ? Bottom : Int)
end
isa(x, DataType) || return Int
(isconcretetype(x) || isprimitivetype(x)) && return _const_sizeof(x)
else
x = widenconst(x)
x !== DataType && isconcretetype(x) && return _const_sizeof(x)
isprimitivetype(x) && return _const_sizeof(x)
end
return Int
end
add_tfunc(Core.sizeof, 1, 1, sizeof_tfunc, 1)
Expand Down
27 changes: 22 additions & 5 deletions base/reflection.jl
Original file line number Diff line number Diff line change
Expand Up @@ -347,15 +347,19 @@ function datatype_alignment(dt::DataType)
return Int(alignment)
end

function uniontype_layout(T::Type)
sz = RefValue{Csize_t}(0)
algn = RefValue{Csize_t}(0)
isinline = ccall(:jl_islayout_inline, Cint, (Any, Ptr{Csize_t}, Ptr{Csize_t}), T, sz, algn) != 0
(isinline, sz[], algn[])
end

# amount of total space taken by T when stored in a container
function aligned_sizeof(T::Type)
@_pure_meta
if isbitsunion(T)
sz = Ref{Csize_t}(0)
algn = Ref{Csize_t}(0)
ccall(:jl_islayout_inline, Cint, (Any, Ptr{Csize_t}, Ptr{Csize_t}), T, sz, algn)
al = algn[]
return (sz[] + al - 1) & -al
_, sz, al = uniontype_layout(T)
return (sz + al - 1) & -al
elseif allocatedinline(T)
al = datatype_alignment(T)
return (Core.sizeof(T) + al - 1) & -al
Expand All @@ -381,6 +385,19 @@ function datatype_haspadding(dt::DataType)
return flags & 1 == 1
end

"""
Base.datatype_nfields(dt::DataType) -> Bool
Return the number of fields known to this datatype's layout.
Can be called on any `isconcretetype`.
"""
function datatype_nfields(dt::DataType)
@_pure_meta
dt.layout == C_NULL && throw(UndefRefError())
return unsafe_load(convert(Ptr{DataTypeLayout}, dt.layout)).nfields
end


"""
Base.datatype_pointerfree(dt::DataType) -> Bool
Expand Down
15 changes: 0 additions & 15 deletions src/codegen.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -3039,21 +3039,6 @@ static bool emit_builtin_call(jl_codectx_t &ctx, jl_cgval_t *ret, jl_value_t *f,
*ret = mark_julia_type(ctx, ctx.builder.CreateMul(len, elsize), false, jl_long_type);
return true;
}
if (jl_is_type_type((jl_value_t*)sty) && !jl_is_typevar(jl_tparam0(sty))) {
sty = (jl_datatype_t*)jl_tparam0(sty);
}
if (jl_is_datatype(sty) && sty != jl_symbol_type &&
sty->name != jl_array_typename &&
sty != jl_simplevector_type && sty != jl_string_type &&
// exclude DataType, since each DataType has its own size, not sizeof(DataType).
// this is issue #8798
sty != jl_datatype_type) {
if (jl_is_concrete_type((jl_value_t*)sty) ||
(jl_field_names(sty) == jl_emptysvec && jl_datatype_size(sty) > 0)) {
*ret = mark_julia_type(ctx, ConstantInt::get(T_size, jl_datatype_size(sty)), false, jl_long_type);
return true;
}
}
}

else if (f == jl_builtin_apply_type && nargs > 0) {
Expand Down
16 changes: 13 additions & 3 deletions test/compiler/inference.jl
Original file line number Diff line number Diff line change
Expand Up @@ -1400,12 +1400,17 @@ egal_conditional_lattice3(x, y) = x === y + y ? "" : 1

using Core.Compiler: PartialStruct, nfields_tfunc, sizeof_tfunc, sizeof_nothrow
@test sizeof_tfunc(Const(Ptr)) === sizeof_tfunc(Union{Ptr, Int, Type{Ptr{Int8}}, Type{Int}}) === Const(Sys.WORD_SIZE ÷ 8)
@test sizeof_tfunc(Type{Ptr}) === Int
@test sizeof_tfunc(Type{Ptr}) === Const(sizeof(Ptr))
@test sizeof_nothrow(Union{Ptr, Int, Type{Ptr{Int8}}, Type{Int}})
@test sizeof_nothrow(Const(Ptr))
@test !sizeof_nothrow(Type{Ptr})
@test !sizeof_nothrow(Type{Union{Ptr{Int}, Int}})
@test sizeof_nothrow(Type{Ptr})
@test sizeof_nothrow(Type{Union{Ptr{Int}, Int}})
@test !sizeof_nothrow(Const(Tuple))
@test !sizeof_nothrow(Type{Vector{Int}})
@test !sizeof_nothrow(Type{Union{Int, String}})
@test sizeof_nothrow(String)
@test !sizeof_nothrow(Type{String})
@test sizeof_tfunc(Type{Union{Int64, Int32}}) == Const(Core.sizeof(Union{Int64, Int32}))
let PT = PartialStruct(Tuple{Int64,UInt64}, Any[Const(10, false), UInt64])
@test sizeof_tfunc(PT) === Const(16)
@test nfields_tfunc(PT) === Const(2)
Expand Down Expand Up @@ -2734,3 +2739,8 @@ end

f_generator_splat(t::Tuple) = tuple((identity(l) for l in t)...)
@test Base.return_types(f_generator_splat, (Tuple{Symbol, Int64, Float64},)) == Any[Tuple{Symbol, Int64, Float64}]

# Issue #36710 - sizeof(::UnionAll) tfunc correctness
@test (sizeof(Ptr),) == sizeof.((Ptr,)) == sizeof.((Ptr{Cvoid},))
@test Core.Compiler.sizeof_tfunc(UnionAll) === Int
@test !Core.Compiler.sizeof_nothrow(UnionAll)

0 comments on commit 004cb25

Please sign in to comment.